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

Local data sans dev server #589

Merged
merged 3 commits into from
Jul 19, 2018
Merged

Conversation

tsibley
Copy link
Member

@tsibley tsibley commented Jul 2, 2018

This restores the ability to run the non-dev server with local data. See the commit message for more details.

This is a more useful value to see than the placeholder /data.
Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Good find - this should definitely be fixed.

Q: which npm script are you using to build the bundles to source local data? I can't see the appropriate one in package.json

Bug: Running npm run build and then npm run server (this is the production build sourcing data from S3, which is what heroku runs) results in the following errors (which are not present on master):

TypeError: path must be absolute or specify root to res.sendFile
    at ServerResponse.sendFile (/Users/james/nextstrain/auspice/node_modules/express/lib/response.js:410:11)
    at app.get (/Users/james/nextstrain/auspice/server.dist.js:301:7)
    at Layer.handle [as handle_request] (/Users/james/nextstrain/auspice/node_modules/express/lib/router/layer.js:95:5)
    at next (/Users/james/nextstrain/auspice/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/Users/james/nextstrain/auspice/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/Users/james/nextstrain/auspice/node_modules/express/lib/router/layer.js:95:5)
    at /Users/james/nextstrain/auspice/node_modules/express/lib/router/index.js:281:22
    at param (/Users/james/nextstrain/auspice/node_modules/express/lib/router/index.js:354:14)
    at param (/Users/james/nextstrain/auspice/node_modules/express/lib/router/index.js:365:14)
    at Function.process_params (/Users/james/nextstrain/auspice/node_modules/express/lib/router/index.js:410:3)

`npm run server localData` (which is `node server.dist.js localData`)
was not working because during the webpack build process the __dirname
reference came to refer to the repo root instead of src/server/.  This
meant the server looked for a data dir two levels up.  In dev mode, the
webpack hot-reloading managed to keep __dirname correct.

This change adjusts the webpack configuration to preserve the __dirname
of the *original* file (src/server/globals.js) instead of using the
dirname of the *packed output* file (server.dist.js).

That config change alone fixes the non-dev server with local data, but
breaks the dev server with local data since the LOCAL_DATA_PATH became
relative instead of absolute.  This necessitated resolving the path to
an absolute one.

__filename is also treated the same way for good measure, although it is
not referred to in our own code.

For more details on the webpack config in play here, refer to:

   https://webpack.js.org/configuration/node/#node-__dirname
@tsibley tsibley force-pushed the local-data-sans-dev-server branch from cecc736 to 1a4bc81 Compare July 17, 2018 15:39
@tsibley
Copy link
Member Author

tsibley commented Jul 17, 2018

Ah, thanks for that catch, @jameshadfield. I've repushed with the fix.

Question for you: What's the status of the Electron build?

Q: which npm script are you using to build the bundles to source local data? I can't see the appropriate one in package.json

I'm building with npm run build and running the server as npm run server localData. The build isn't parameterized on local/remote data; just the running server. (This is as it was, I just made it work again.)

@trvrb
Copy link
Member

trvrb commented Jul 18, 2018

Question for you: What's the status of the Electron build?

Almost certainly broken.

@tsibley
Copy link
Member Author

tsibley commented Jul 19, 2018

In that case I won't try to determine if path.join in index.js (used by Electron) needs to be path.resolve. (Thought I don't think it does.)

@tsibley
Copy link
Member Author

tsibley commented Jul 19, 2018

@jameshadfield Ping. This is ready for re-review.

@jameshadfield jameshadfield merged commit 1651d83 into master Jul 19, 2018
@jameshadfield jameshadfield deleted the local-data-sans-dev-server branch July 19, 2018 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants