Skip to content
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.

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

Auspice dev server broken #616

Open
jameshadfield opened this issue Oct 25, 2022 · 5 comments
Open

Auspice dev server broken #616

jameshadfield opened this issue Oct 25, 2022 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@jameshadfield
Copy link
Member

jameshadfield commented Oct 25, 2022

Nextstrain.org uses a customised version of auspice, typically built via ./build.sh auspice or similar. We also allow the use of auspice's dev server in order to develop these customisations. This dev server is provided with both client-side customisation code as well as custom endpoints to handle the API requests made from the auspice client so that we can mimic nextstrain.org behavior. (By the way - these endpoints in ./src/endpoints/charon/index.js were specifically exposed in a single file to allow this; see Supplying custom handlers to the Auspice server for background.)

Since our migration from CJS to ESM (PR #583 and others) the dev server no longer works:

$ cd auspice-client
$ npx auspice develop --verbose --extend ./customisations/config.json --handlers ../src/endpoints/charon/index.js
...
/Users/enigma/projects/nextstrain/nextstrain.org/node_modules/auspice/cli/view.js:48
    const inject = require(handlersPath); // eslint-disable-line
                   ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/enigma/projects/nextstrain/nextstrain.org/src/endpoints/charon/index.js from /Users/enigma/projects/nextstrain/nextstrain.org/node_modules/auspice/cli/view.js not supported.
Instead change the require of index.js in /Users/enigma/projects/nextstrain/nextstrain.org/node_modules/auspice/cli/view.js to a dynamic import() which is available in all CommonJS modules.
    at loadAndAddHandlers (/Users/enigma/projects/nextstrain/nextstrain.org/node_modules/auspice/cli/view.js:48:20)
    at Object.run (/Users/enigma/projects/nextstrain/nextstrain.org/node_modules/auspice/cli/develop.js:74:18)
    at Object.<anonymous> (/Users/enigma/projects/nextstrain/nextstrain.org/node_modules/auspice/auspice.js:37:11) {
  code: 'ERR_REQUIRE_ESM'
}

Temporary solution

Branching off 7737fad (the last commit before the CJS to ESM conversion) allows the dev server to work. Working from this should be ok for the ideas I'm testing out at the moment.

Long term Solution

I imagine this is going to be solved by changing the auspice server code, as that's what's importing the handlers. Ideally this would be a backwards compatible change. @victorlin is it possible to allow importing CJS and ESM code?

@jameshadfield jameshadfield added the bug Something isn't working label Oct 25, 2022
@jameshadfield
Copy link
Member Author

jameshadfield commented Oct 25, 2022

I spoke too soon regarding the temporary solution above. There are other broken components beyond the CJS to ESM conversion. Attempting to load datasets results in a (charon) API call which fails as our handlers are now designed in such a way that they rely on middleware and thus don't work when running the dev server:

$ npx auspice develop --verbose --extend ./customisations/config.json --handlers ../src/endpoints/charon/index.js
...
webpack built 20d2c4ca20aeb4778e80 in 33448ms

Getting (nextstrain) datasets for: prefix=zika
[warning]       Failed to fetch v2 main JSON: TypeError: Cannot read properties of undefined (reading 'dataset')
/Users/enigma/projects/nextstrain/nextstrain.org/src/endpoints/sources.js:113
  sendSubresource(req => req.context.dataset.subresource(type));
                                     ^

TypeError: Cannot read properties of undefined (reading 'dataset')
    at /Users/enigma/projects/nextstrain/nextstrain.org/src/endpoints/sources.js:113:38

@jameshadfield jameshadfield changed the title Auspice dev server broken since ESM migration Auspice dev server broken Oct 25, 2022
@victorlin victorlin self-assigned this Oct 25, 2022
@victorlin
Copy link
Member

victorlin commented Oct 25, 2022

Another temporary solution to the CJS/ESM issue is to use Node.js v14. A warning is shown but the dev server still works.

EDIT: I also spoke too soon... I had something else running in the background thinking it was working. The output makes it look like it does, but it hangs and does not actually start the server.

[verbose]	Loading handlers from /Users/vlin/repos/nextstrain/nextstrain.org/src/endpoints/charon/index.js
Must use import to load ES Module: /Users/vlin/repos/nextstrain/nextstrain.org/src/endpoints/charon/index.js
require() of ES modules is not supported.
require() of /Users/vlin/repos/nextstrain/nextstrain.org/src/endpoints/charon/index.js from /Users/vlin/repos/nextstrain/nextstrain.org/node_modules/auspice/cli/view.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/vlin/repos/nextstrain/nextstrain.org/package.json.

[verbose]	Generating Babel Config
...

@victorlin
Copy link
Member

As you mentioned, this comes down to Auspice's CJS code trying to require() ESM code from nextstrain.org. I'm still trying to wrap my head around the handlers and when they are used (docs are helpful!) because I'm a bit confused why --handlers is only necessary for auspice develop.

Based on nodejs/modules#454, I don't think what we're looking to do is supported. Two long-term options:

  1. Revert Convert top-level project from CJS to ESM #583 so nextstrain.org is a CJS project.
  2. Make Auspice code portable to both CJS and ESM. This seems possible from some quick searching, but might take some time to get things working properly.

@jameshadfield
Copy link
Member Author

I'm still trying to wrap my head around the handlers and when they are used

A way of explaining this is to compare it with our normal nextstrain.org usage. There we first have a build stage to compile the auspice client (with customisations) into an index.html entrypoint and some JS bundles, a process which uses ./build.sh which uses auspice build to do the compiling. We then run our server (./server.js) which sets up a bunch of API handlers (e.g. handing these APIs which will come from the auspice client) and also responds to certain GET requests by sending the auspice HTML entrypoint, which in turn uses those JS bundles we've built (how we actually send this is somewhat complicated as it's part of a large middleware chain, but this is where it happens).

For developing the auspice client in the context of nextstrain.org, we want to incrementally update the client code as we change things rather than rebuilding it (⌛), and auspice develop does just this. However that dev-server by itself will use the native auspice API handlers which read datasets & narratives from disk rather than the API handlers we use here which can access datasets and narratives on S3, GitHub etc. For this reason I built the auspice server with the ability to use custom handlers, and built the nextstrain.org handlers in such a way that auspice can import them.

The nextstrain.org server has come a long way since I first built it, and it may not make sense to use this development direction as (a) we've moved to ESM and (b) our API handlers are much more complex than the ones an auspice server expects. We also don't use auspice cusomisations as much as we used to, as we now route more pages to Gatsby whereas previously we would use the (custom) auspice splash page. Regardless, we should have a nicer path to auspice development work than we do now where one has to rebuild the auspice client each time we want to test a change.

@tsibley
Copy link
Member

tsibley commented Oct 27, 2022

I'd recommend taking the same approach I used to integrate the Gatsby dev server into the nextstrain.org codebase.

The gist is that we'd run auspice develop as a sidecar server and the nextstrain.org server would proxy to it as appropriate instead of sending the pre-built static Auspice entrypoint and assets. This would require that the client-side code served by auspice develop makes Charon requests relative to the current browser page/URL rather than the host:port it's running as. I suspect that might already be the case, though?

# Starts "gatsby develop" on localhost:8000
(cd static-site && npm run develop) &
# Start Express server with proxying of Gatsby assets to localhost:8000
NODE_ENV="dev" GATSBY_DEV_URL=http://localhost:8000 node server.js --verbose

app.locals.gatsbyDevUrl = PRODUCTION ? null : process.env.GATSBY_DEV_URL;

nextstrain.org/src/app.js

Lines 422 to 444 in b68e829

/* Gatsby static HTML pages and other assets.
*
* Any URLs matching Gatsby's static files will be handled here, e.g.
* static-site/public/influenza/index.html will be served for /influenza.
*
* When a Gatsby dev server is in use, assets are proxied from the dev server
* instead of served straight from disk.
*/
if (app.locals.gatsbyDevUrl) {
/* eslint-disable-next-line import/no-extraneous-dependencies */
const {createProxyMiddleware} = await import("http-proxy-middleware");
// WebSocket endpoint
app.get("/socket.io/", createProxyMiddleware(app.locals.gatsbyDevUrl, { ws: true }));
/* This is the end of the line in gatsbyDevUrl mode, as the proxy middleware
* doesn't fallthrough on 404 and even if it did the Gatsby dev server
* returns a 200 Ok for missing pages.
*/
app.use(createProxyMiddleware(app.locals.gatsbyDevUrl));
} else {
app.use(endpoints.static.gatsbyAssets);
}

/**
* Generate Express handler that sends the specified Gatsby page.
*
* Note that Gatsby may perform client-side routing if the browser location
* (i.e. the current request path) doesn't match the page served (i.e. the
* page's `path` and/or `matchPath` configuration).
*
* If the application is in `gatsbyDevUrl` mode, pages are transparently
* sourced from the configured `gatsby develop` server instead of the
* filesystem.
*
* @param {String} page - Path to a Gatsby page's rendered static HTML file
* @returns {asyncExpressHandler}
*/
const sendGatsbyPage = (page) => async (req, res) => {
if (req.app.locals.gatsbyDevUrl) {
const pageUrl = (new URL(page, req.app.locals.gatsbyDevUrl)).toString();
/* eslint-disable-next-line import/no-extraneous-dependencies */
const proxy = (await import("http-proxy")).createProxyServer({
target: pageUrl,
ignorePath: true, // ignore req.path since pageUrl is fully specified
});
utils.verbose(`Sending Gatsby page ${pageUrl} for ${req.originalUrl}`);
return new Promise((resolve, reject) => {
// eslint-disable-next-line no-shadow
proxy.on("end", (req, res) => {
res.end();
resolve();
});
proxy.web(req, res, (err) => reject(err));
});
}
utils.verbose(`Sending Gatsby page ${page} for ${req.originalUrl}`);
return await sendFile(res, gatsbyAssetPath(page));
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Prioritized
Development

No branches or pull requests

3 participants