feat: use server.enhanceMiddleware from custom metro configuration

[isnotgood]

Summary:

I want to customize my Metro server with server.enhanceMiddleware in my metro.config.js. Currently runServer.js:82 overriddes this configuration option. PR enables usage of custom enhanceMiddleware from metro configuration for react-native-cli.

Test Plan:

Add this to your rn-cli.config.js or metro.config.js to clean react native project:

const assert = require('assert');

module.exports = {
  /* ... */
  server: {
    /* ... */
    enhanceMiddleware: (middleware, server) => {
      assert.equal(typeof middleware, 'function');
      assert.equal(server.constructor.name, 'Server');
      console.log('Middleware enhanced');

      return (req, res, next) => {
        assert.equal(req.constructor.name, 'IncomingMessage');
        assert.equal(res.constructor.name, 'ServerResponse');
        assert.equal(typeof next, 'function');
        assert.equal(next.name, 'next');
        console.log('Enhanced middleware used for: ' + req.url);

        return middleware(req, res, next);
      }
    }
  },
};

Steps:

• 1️⃣ Start server -> Observe server starts and prints "Middleware enhanced" and doesn't throw any error.
• 2️⃣ Request /index.bundle?platform=ios -> Observe server prints "Enhanced middleware used for: /index.bundle?platform=ios" and returns bundle properly without throwing error.

$ react-native start
┌──────────────────────────────────────────────────────────────────────────────┐
│                                                                              │
│  Running Metro Bundler on port 8081.                                         │
│                                                                              │
│  Keep Metro running while developing on any JS projects. Feel free to        │
│  close this tab and run your own Metro instance if you prefer.               │
│                                                                              │
│  https://github.com/facebook/react-native                                    │
│                                                                              │
└──────────────────────────────────────────────────────────────────────────────┘

1️⃣ Middleware enhanced
Looking for JS files in
   /Users/lukasweber/dev/test-rn-project/

Loading dependency graph, done.
2️⃣ Enhanced middleware used for: /index.bundle?platform=ios
 BUNDLE  [ios, dev] ./index.js ▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓ 100.0% (2/2), done.

[thymikee]

LGTM

[grabbou]

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.

[grabbou]

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.

[isnotgood]

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.

[isnotgood]

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?

[grabbou]

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.

[grabbou]

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.

[grabbou]

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 :)

[isnotgood]

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).

[isnotgood]

@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.

[grabbou]

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?

…

On Wed, 21 Aug 2019 at 15:41, Lukas Weber ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/cli/src/commands/server/runServer.js <#614 (comment)> : > @@ -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); + }; 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). — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#614?email_source=notifications&email_token=AASZZRSMYY4K2HTGWUG627TQFVAXDA5CNFSM4IJE3ACKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCHOW7A#discussion_r316190533>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASZZRS7BC5MCJR3IKLHSNLQFVAXDANCNFSM4IJE3ACA> .