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

Build: Restart server on file change #848

Closed
wants to merge 2 commits into from

Conversation

seear
Copy link
Contributor

@seear seear commented Nov 26, 2015

At the moment, changes to a file used on the server, including shared modules, requires the Node server to be restarted. Now that we are starting to use more shared modules, an automatic compile/restart becomes more important.

This PR changes the run target so that it:

Runs webpack --watch on server files in the background to compile on change.
Uses nodemon to restart server after compilation.

To Test
make clean
make run
Make a change to any file used on the server and check that the server restarts and picks up the change

@seear seear added the Build label Nov 26, 2015
@seear seear self-assigned this Nov 26, 2015
@seear seear added this to the Themes: Showcase M3-LoggedOut milestone Nov 26, 2015
@lamosty
Copy link
Contributor

lamosty commented Nov 26, 2015

I'm just testing this and after running make clean and make run, I'm getting this error:

It's probably the "`" character it has problems with (https://github.com/Automattic/wp-calypso/blob/try/restart-server-on-change/server/bundler/index.js#L55). Interesting that it works in the master branch.

@seear
Copy link
Contributor Author

seear commented Nov 26, 2015

It's probably the "`" character it has problems with

Great, thanks for spotting that, @lamosty. The nodemon command was not passing the transpiled bundle to node, hence node was tripping up on the ES6 "`" syntax.

Keep your eye open for the [Status] Needs Review labels on PRs, which are generally further along in development and need more eyes on them: https://github.com/Automattic/wp-calypso/blob/master/CONTRIBUTING.md#lifecycle-of-a-pull-request

@lamosty
Copy link
Contributor

lamosty commented Nov 26, 2015

@seear Thanks. Ah, that was it.

Alright, I'll do that. Thanks for the info.

@seear seear added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Nov 26, 2015
Also use Nodemon to restart server after compilation.
@seear seear force-pushed the try/restart-server-on-change branch from 6258e17 to 9a87d96 Compare December 4, 2015 10:57
@seear
Copy link
Contributor Author

seear commented Dec 4, 2015

Running webpack with & means that it won't stop with a simple ctrl+c.

@seear seear added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 4, 2015
Nodemon watches server and shared folders and restarts both
compilation and the node server on a change.
@seear
Copy link
Contributor Author

seear commented Dec 17, 2015

I don't believe that this is currently a desirable change. Because the client JS is built by the Node server, and there is no way to cache the compiled JS to disk, restarting the Node server means a long wait (~46 seconds on my machine) before the system is running again.

We should revisit this PR once we have a better solution for marking components as useable server-side (see #1594).

@seear seear closed this Jan 6, 2016
@lancewillett lancewillett deleted the try/restart-server-on-change branch January 14, 2016 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants