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

Avoid restarting server in development when changed file(s) not used by server bundle. #10686

Merged
merged 4 commits into from Sep 6, 2019

Conversation

@benjamn
Copy link
Member

benjamn commented Sep 5, 2019

Most importantly, this change means that changes to files not used by the server bundle will not trigger a server restart.

Fixes #10449 by implementing the strategy I described in this comment: #10414 (comment)

@benjamn benjamn added this to the Release 1.8.2 milestone Sep 5, 2019
@benjamn benjamn self-assigned this Sep 5, 2019
@benjamn benjamn force-pushed the conservative-server-restart branch from e3631c7 to 98d3ace Sep 5, 2019
Most importantly, this change means that changes to files not used by the
server bundle will not trigger a server restart.

Fixes #10449 by implementing the strategy I described in this comment:
#10414 (comment)
@benjamn benjamn force-pushed the conservative-server-restart branch from 98d3ace to 4084848 Sep 5, 2019
benjamn added 2 commits Sep 6, 2019
The previous implementation simply avoided calling watchSet.addFile for
potentially unused files, trusting that addFile would be called later if
the file was eventually used. However, this strategy left the contents of
watchSet.files incomplete for tasks such as IsopackCache._checkUpToDate,
which require full information about all files, even the ones that might
not be used by the bundle. The new strategy maintains metadata about
potentially unused files in a separate data structure, which will be
merged/cloned/serialized/deserialized along with other WatchSet data.
Part of #10686.
@benjamn benjamn changed the title Add files to unibuild.watchSet only if needed by unibuild.arch. Avoid restarting server in development when changed file(s) not used by server bundle. Sep 6, 2019
@benjamn benjamn merged commit 3c049a8 into release-1.8.2 Sep 6, 2019
19 checks passed
19 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Clean Up Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Isolated Tests Your tests passed on CircleCI!
Details
ci/circleci: Test Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@benjamn benjamn deleted the conservative-server-restart branch Sep 6, 2019
@pmogollons

This comment has been minimized.

Copy link
Contributor

pmogollons commented Sep 7, 2019

@benjamn this is an awesome improvement. Got the full page reload time from 8s to 5s.

Is there a way to be able to include something like this in a module that is loaded on server and client and still get the benefits?

// /startup/both/index.js

import { Meteor } from 'meteor/meteor';

if (Meteor.isProduction) {
  require('./routes');
}

import './slingshot';
import './registerApi';

Just having there the require or the import makes the server to restart even if not actually imported.

@benjamn

This comment has been minimized.

Copy link
Member Author

benjamn commented Nov 7, 2019

@pmogollons Great to hear it's working so well!

I would love your help testing a slightly simpler implementation of this functionality, coming soon (probably in 1.8.2-rc.7), once #10763 is merged.

To answer your question, in Meteor 1.8.3, we want to tackle tree shaking / dead code elimination, which involves pruning the dependency tree while scanning imports in the ImportScanner. I believe it should be possible to treat values like Meteor.isProduction as constants during this process, and eliminate those branches if their conditions are false, so that the ./routes dependency would not show up at all in the production bundle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.