Skip to content

feat: use server.enhanceMiddleware from custom metro configuration #614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 21, 2019
Merged

feat: use server.enhanceMiddleware from custom metro configuration #614

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/cli/src/commands/server/runServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,15 @@ async function runServer(argv: Array<string>, ctx: ConfigT, args: Args) {
middlewareManager.serveStatic.bind(middlewareManager),
);

metroConfig.server.enhanceMiddleware = middleware =>
middlewareManager.getConnectInstance().use(middleware);
const customEnhanceMiddleware = metroConfig.server.enhanceMiddleware;

metroConfig.server.enhanceMiddleware = (middleware, server) => {
if (customEnhanceMiddleware) {
middleware = customEnhanceMiddleware(middleware, server);
}

return middlewareManager.getConnectInstance().use(middleware);
};
Copy link
Member

@grabbou grabbou Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering one thing - if this option is available in the Metro documentation, how does it work when you run it directly, not via React Native CLI, where we have this "custom logic" to handle this option?

The reason I am asking is this: I think Metro handles this option natively and there's no need to have this code at all.

Copy link
Member

@grabbou grabbou Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we could use this PR (or maybe fill in another) as an opportunity to actually refactor this logic up a bit.

We already have our server (it's called MiddlewareManager which is wrong, because in reality, it's a server). We could use createMiddleware(https://github.com/facebook/metro/blob/5ac712e1fba82ea0da2931f9ad732a82ac04bccb/docs/API.md#createconnectmiddlewareconfig-options) option from Metro to just do server.use(metro(config)) which would be a good step towards enabling other bundlers too.

I don't think we need to handle watchFolders ourselves either and this might be a leftover from Metro extraction.

Copy link
Contributor Author

@isnotgood isnotgood Aug 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, if we use Metro.createConnectMiddleware() everything will work out of box so we don't need this code at all. I can take a stab at it and let's see if it will work out.

Copy link
Contributor Author

@isnotgood isnotgood Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we would have to basically copy Metro.runServer() ...and then keep parity with what FB folks will do there in the future. Are you sure that's what you want?

Copy link
Member

@grabbou grabbou Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, Metro.runServer resolves with an http server -> https://github.com/facebook/metro/blob/416767c51ef99d594cbde64ac41379b57e45c667/packages/metro/src/index.js#L201, so I believe we could try using that one for time being and just add our middlewares on top of it.

Copy link
Member

@grabbou grabbou Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going forward, it would be great to 'rewrite' that piece to exist within our codebase, but it's not a priority for now and will help us greatly reduce the code.

Copy link
Member

@grabbou grabbou Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I would suggest is let's merge your "quick fix" as is without blocking the PR and then... submit a follow-up with a proper refactor of this piece?

It would be great to create a special issue where we can track the progress.

Let me know if this sounds like something you'd like to take care of :)

Copy link
Contributor Author

@isnotgood isnotgood Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can get httpServer started by Metro, we can hackily do something like this

const httpServer = await Metro.runServer();

httpServer.listeners('request').forEach(listener => {
  httpServer.removeListener('request', listener); // remove listeners added by Metro
  middlewareManager.getConnectInstance().use(listener); // move them to our connect instance
});

server.addListener('request', middlewareManager.getConnectInstance());

This way we can add anything into chain before the Metro's middleware will kick in. It could make code a little bit cleaner on CLI's side (no need to use enhanceMiddleware to hook in).

Copy link
Contributor Author

@isnotgood isnotgood Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grabbou sry, didn't seen the following messages after #614 (comment)

Let me know if this sounds like something you'd like to take care of :)

This is functionality I'll find great use for on my day job, so definitely something I want to contribute to.

Or.. we could just do httpServer.use(ourMiddlewareA) and so on, adding them all on top of that. My only worry is that there might be conflicting middlewares, but I am not that into this codebase to be able to tell whether this is valid concern or not. Got any ideas?
…

I'm afraid that httpServer returned by Metro.runServer() is plain node's http.Server which doesn't have concept of middleware baked in, that's connect/express thing.


const serverInstance = await Metro.runServer(metroConfig, {
host: args.host,
Expand Down