[legacy-framework] Add support for async experimental.initServer()

[g3offrey]

Closes: blitz-js/legacy-framework#279

What are the changes and their implications?

I did the solution proposed by @flybayer on the issue blitz-js/legacy-framework#279

To do so I had to load the config before the server is listening. Indeed today, the config loading is done at first on prepare method, which is run on the server listening.

The loadConfig method was private so I had to set it a public 😢
Another drawback of this is that the loadConfig is now done two times: before the listening and when we call getServer (at the listening).

I was thinking about two things doing that, moving the initServer logic inside a NextServer method (onServerInit ?), so I can set back loadConfig as private.

Or maybe instead of calling loadConfig, I can call getServer (setting it public before so) which has a loadConfig inside of it. This has advantages of preparing the serverPromise per advance and won't do a duplicate call to loadConfig.

What do you think ? 🤔

(As this is an experimental feature at the moment, we can log a refactoring ticket and do the changes on the future.)

Bug Checklist

• Integration test added (see test docs if needed)

Feature Checklist

• Integration test added (see test docs if needed)
• Documentation added/updated (submit PR to blitzjs.com repo)

[flybayer]

@g3offrey thank you so much!

Upon review, I think we could run initServer just before this line in getServer. What do you think?

[g3offrey]

@flybayer Don't worry, I'm glad to contribute to such a project 😄
Sure, I think this is possible, however I have to do some modification to do so.

First, at the moment the getServer method is called first on prepare, so after listening. (This is not what we want)

With my modification, we can call it once in getRequestHandler instead of at every request (in request handler).
Also this method is called before the listening so it is perfect for our use-case.

 async getRequestHandler() {
    const requestHandler = await this.getServerRequestHandler()

    return (
      req: IncomingMessage,
      res: ServerResponse,
      parsedUrl?: UrlWithParsedQuery
    ) => {
      return requestHandler(req, res, parsedUrl)
    }
  }

As now getRequestHandler is simply a wrapper to getServerRequestHandler. Maybe in the future we can delete this method and call directly getServerRequestHandler or move content from one to another. I had to change all the calls as now this method is async.

With these modifications, I can implement your solution. What do you think? Do you prefer this way of doing things or the previous one? 😄

Check out the last commit. I hope I am clear enough, that's not easy by text 😅

[flybayer]

Hey @g3offrey so sorry for then long delay here. I really appreciate your work on this. I spent some time analyzing this, and I think we were following a constraint that we don't need. I can't see any reason that we have to await initServer before the http server is listening. Because initServer can't change the listening port or anything.

So I think we can just add await initServer in getServer without all the other changes. What do you think?

And in general, we want to have a solution that minimizes changes since we are a fork of nextjs and merge in the their changes.

[g3offrey]

Hi @flybayer

Don't worry, it seems to be a big piece of changes, so I totally understand that it is long to review.
You know more information than me on Blitz codebase, so I am totally fine with your solution.

I think I implemented it on my latest commit (after reverting previous code). Please check it out and tell me if any changes are required 😄

[flybayer]

Awesome, thank you so much!!