Amended changes from #7452 #11897
Amended changes from #7452 #11897
Conversation
…dleware, obsoleted old namespace, Added Generic UseDevelopmentServer and Vue UseVueDevelopmentServer.
- Removed VueDevelopmentServer - Added Dictionary for arguments to DevelopmentServer - Unobseleted ReactDevelopmentServer
elken
requested review from
analogrelay,
ryanbrandenburg and
Tratcher
as code owners
|
|
| }; | ||
| var npmScriptRunner = new NpmScriptRunner( | ||
| sourcePath, npmScriptName, null, envVars); | ||
| sourcePath, npmScriptName, null, envVars.Union(extraArgs)); |
There was a problem hiding this comment.
I'm aware this will cause a NullReference. I'll setup a local build env and possibly add some tests.
mkArtakMSFT
added
area-blazor
|
Thanks for your effort, @elken. |
|
I'll take a look in the office tomorrow
Regards,
Ellis Kenyő
GitLab | GitHub | Keybase | elken.me
…On 8 Jul 2019, 16:12, at 16:12, Artak ***@***.***> wrote:
<p>Thanks for your effort, <a class="user-mention"
data-hovercard-type="user"
data-hovercard-url="/hovercards?user_id=2872862"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
***@***.***</a>.<br>
Seems like you have some merge conflicts. Can you please resolve those,
while we're looking into this?</p>
<p
style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br
/>You are receiving this because you were mentioned.<br />Reply to this
email directly, <a
href="#11897
it on GitHub</a>, or <a
href="https://github.com/notifications/unsubscribe-auth/AAV5MHXR3Y4OKOKR3HSW2C3P6NKOZANCNFSM4H525DHQ">mute
the thread</a>.<img
src="https://github.com/notifications/beacon/AAV5MHTE4WHR6EY2KS7B7E3P6NKOZA5CNFSM4H525DH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZNMW5A.gif"
height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
***@***.***": "http://schema.org",
***@***.***": "EmailMessage",
"potentialAction": {
***@***.***": "ViewAction",
"target":
"#11897",
"url":
"#11897",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
***@***.***": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>
|
|
I've attempted to fix said conflicts in 32321bb |
| { | ||
| internal static class ReactDevelopmentServerMiddleware | ||
| internal static class DevelopmentServerMiddleware |
There was a problem hiding this comment.
Looks like the file path and filename should be updated to match this.
| string npmScriptName, | ||
| string waitText, | ||
| Dictionary<string, string> extraArgs, | ||
| string serverName = "App") |
There was a problem hiding this comment.
Does there have to be a default value for this parameter? It would be clearer to make it mandatory if that's possible.
|
|
||
| var envVars = new Dictionary<string, string> | ||
| { | ||
| { "PORT", portNumber.ToString() }, | ||
| { "BROWSER", "none" }, // We don't want create-react-app to open its own extra browser window pointing to the internal dev server port | ||
| { "PORT", portNumber.ToString() } |
There was a problem hiding this comment.
It's problematic to assume that all SPA development servers would use an environment variable to configure this, or that they would be looking for an environment variable with this particular name.
| }; | ||
|
|
||
| waitText = waitText.Replace("$PORT", portNumber.ToString()); |
There was a problem hiding this comment.
Having a special custom syntax to define the message as a function of the port number is very nonstandard and makes me think we should reconsider this layering.
|
Thanks for submitting this update. Based on this, my preference would be not to have an API at this level of abstraction, because it's simultaneously quite coupled to assumptions around React dev server (e.g., how the port is specified), while also not being coupled enough to safely hardcode this knowledge like I think it would be better to publish this as a separate package, not as part of ASP.NET Core itself. That would allow much more flexibility to make whatever assumptions you want, and to change (with breaking changes) any time you need based on changes in the 3rd-party SPA frameworks and their associated tooling. Sorry that's an annoying thing to hear after you've been working on this! I definitely appreciate your efforts to contribute this, and how you've amended the PR following feedback. I hope it makes sense that we're trying to pick only APIs that can be supported in the long term and don't create exposure to significant risks that they cease to be supportable due to changes we can't control. cc @danroth27 for any further thoughts on this. |
|
Understand the thought behind, I have been rolling my own impl similar to the deployed one for a few of our internal packages so I'll roll that up into a nuget package tmrw. The thought process behind the custom syntax for $PORT was my own solution to that problem, as the particular stack we use (NextJS) reads the port via cmd line flags passed to the npm/yarn task, and is also included in the "waiting" output. |
|
Thanks @elken! |
RE: "over-specialised" APIs, I agree with the idea that the ready signal might not be static text, but I think digging into that rabbit hole could lead to any number of implementations and all major frameworks do indeed return static text on success.