Adding a base npm middleware and making it public

[alexeyzimarev]

Summary of the changes (Less than 80 chars)

• Added a more generic npm middleware and making it public
• Changed the Angular CLI and React dev server middleware to use the new npm middleware
• The change makes it possible to create a custom middleware for other SPA frameworks, for example, Vue.js

Addresses #5214 (partially)

[dnfclas]

All CLA requirements met.

[alexeyzimarev]

If we get a generic npm server middleware, adding a new Vue CLI middleware would be trivial, check https://gist.github.com/alexeyzimarev/deda49dc8faf26b0d0978610d91b5c41

It also simplifies the code for the existing Angular and React middleware classes.

[alexeyzimarev]

I don't the build fails due to my changes

[Tratcher]

/AzurePipelines run AspNetCore-ci

[azure-pipelines]

Success! 2 created and queued.

[SteveSandersonMS]

I'm fine with doing this sort of thing, but I do think the APIs this creates are very unusual.

        public static void Attach(ISpaBuilder spaBuilder,
            string npmScriptName,
            Func<int, string> npmParameters,
            Func<int, IDictionary<string, string>> envVars, 
            Regex outputMatch, Func<Match, int, Uri> listeningUri)

We don't usually have people pass things like Func<int, string>. It's not obvious for the caller what their callback is meant to do here, or what the int param is (I know it's the port, but I don't think it's obvious). It's also a bit sketchy to assume that all SPA server middleware is always going to make its "am I ready or not" decision based on a Regex match of some console process's output - that seems pretty specific to the way that Angular CLI and create-react-app work.

Perhaps rather than bundling all these assumptions together in one method, we should have a collection of separate methods you can pick the relevant ones out of, e.g. on some NpmHelpers class:

• Process StartNpmScript(string scriptName, string arguments)
• IAsyncEnumerable<string> ReadOutputLinesAsync(Process process)
• Task PollForSuccessResponseAsync(Uri uri, TimeSpan timeout)

Then AngularCliMiddleware, ReactDevelopmentServerMiddleware, and external SPA dev server middlewares can be constructed more simply out of these primitives.

@ryanbrandenburg What's your preference on how the SPA middleware APIs should evolve?

[alexeyzimarev]

We can have named delegates instead and it will be more obvious. At the same time, I am not a big fan of coupling to npm, there’s an open issue about yarn and it might be a good think to address that too. But I saw a comment that the npm script runner will be moved to the Node Services package and it wasn’t, so knowing the thoughts of moving forward would definitely help.

On Fri, 1 Feb 2019 at 13:38, Steve Sanderson ***@***.***> wrote: I'm fine with doing this sort of thing, but I do think the APIs this creates are very unusual. public static void Attach(ISpaBuilder spaBuilder, string npmScriptName, Func<int, string> npmParameters, Func<int, IDictionary<string, string>> envVars, Regex outputMatch, Func<Match, int, Uri> listeningUri) We don't usually have people pass things like Func<int, string>. It's not obvious for the caller what their callback is meant to do here, or what the int param is (I know it's the port, but I don't think it's obvious). It's also a bit sketchy to assume that all SPA server middleware is always going to make its "am I ready or not" decision based on a Regex match of some console process's output - that seems pretty specific to the way that Angular CLI and create-react-app work. Perhaps rather than bundling all these assumptions together in one method, we should have a collection of separate methods you can pick the relevant ones out of, e.g. on some NpmHelpers class: - Process StartNpmScript(string scriptName, string arguments) - IAsyncEnumerable<string> ReadOutputLinesAsync(Process process) - Task PollForSuccessResponseAsync(Uri uri, TimeSpan timeout) Then AngularCliMiddleware, ReactDevelopmentServerMiddleware, and external SPA dev server middlewares can be constructed more simply out of these primitives. @ryanbrandenburg <https://github.com/ryanbrandenburg> What's your preference on how the SPA middleware APIs should evolve? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7046 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsMVZ2l2MkvoKWUHE5e9d41VUHanQ-Uks5vJDVIgaJpZM4aUtp7> .

-- Med vennligst hilsen / Best regards, Alexey V. Zimarev

[mkArtakMSFT]

@SteveSandersonMS does this look align with our long-term vision in this area? The issue this is partially resolving is our plan (#5182). If this looks fine, let's ask @alexeyzimarev to rebase this so we can get this merged for 5.0.

[SteveSandersonMS]

@mkArtakMSFT As I commented above, I think we certainly could add a base NPM middleware. My concern is that the APIs proposed in this PR aren't what we would want, for the reasons described in my comment. I suggested some alternate API design thoughts in my comment.

[mkArtakMSFT]

Thanks for your effort, @alexeyzimarev.
After some further discussion we've decided that we don't want to take this PR as it's not a complete solution for #5182. We will instead come up with a design for the feature and tackle it then.