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

Run all "bare" package files before requiring eager entry point modules. #8972

Merged
merged 2 commits into from
Aug 7, 2017

Conversation

benjamn
Copy link
Contributor

@benjamn benjamn commented Jul 31, 2017

Immediately after each Meteor package is evaluated, we synchronously require its eager entry point modules (typically those identified by api.mainModule), before loading other packages. This behavior makes the load order of packages more important than it should be, leading to bugs like #8969, where one package synchronously imports another package that hasn't loaded yet, and gets weird/undefined results.

The natural solution here is to run all eager require calls after loading all package code. However, we have avoided making that change because of an obscure feature that allows package files to be bare, which means they are not wrapped as modules, and thus may declare variables directly in the package scope. Historically, bare code could run in between eager require calls, according to the order of api.addFiles calls in the package.js file.

If we care about the intentional interleaving of bare code with require calls, then calling require later is not possible without breaking backwards compatibility, since we can't defer the bare code without putting it a different scope. However, it appears that no Meteor core packages depend on bare/require interleaving, and I suspect very few existing Meteor packages will be broken by this change.

Specifically, this PR just runs all bare code in a given package before calling require to evaluate the package's eager entry point module(s). If this works, then we're one step closer to deferring all require calls until every package has finished evaluating, which will fix bugs like #8969, as well as making it unnecessary to move files from node_modules into the modules package, which has been a source of some confusion ever since Meteor 1.3.

@benjamn benjamn added this to the Release 1.5.2 milestone Jul 31, 2017
@benjamn benjamn self-assigned this Jul 31, 2017
@benjamn benjamn requested review from abernix and hwillson July 31, 2017 23:55
@benjamn
Copy link
Contributor Author

benjamn commented Jul 31, 2017

Note to self:

  • add a note about this to History.md.

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

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

LGTM. Most every bare usage I've seen have been npm-wrapper (read: legacy, discouraged) Meteor packages which do something along the lines of api.addFiles('jquery_or_some_other_library.min.js', { bare: true }).

I believe those will continue to work fine even with this change.

Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Looks great! One small comment question.

@@ -398,13 +398,22 @@ _.extend(Module.prototype, {
// Now that we have installed everything in this package or
// application, immediately require the non-lazy modules and
// evaluate the bare files.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this comment around a bit, maybe to something like?

// Now that we have installed everything in this package or
// application, first evaluate the bare files, then require the 
// non-lazy modules.

@benjamn benjamn force-pushed the run-bare-files-before-eager-modules branch from 910da1b to a886f06 Compare August 2, 2017 19:16
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.

None yet

3 participants