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

Rely on npm babel-runtime instead of Meteor helper implementations #7995

Merged
merged 20 commits into from Nov 2, 2016

Conversation

benjamn
Copy link
Contributor

@benjamn benjamn commented Oct 31, 2016

The more I've thought about the underlying issues involved in #7956, the more I believe the time has come for Meteor to stop providing custom implementations of babel-runtime/helpers/* and instead simply rely on the npm babel-runtime package.

This pull request adds babel-runtime to the default package.json file for new apps (a la meteor-node-stubs), installs those basic packages when running meteor test-packages (which is something we should have been doing all along), simplifies the Meteor babel-runtime package (it now throws an error if babel-runtime has not been installed), and cleans up some other logic that no longer matters.

I'm torn about whether to include this in 1.4.2.1 or wait for 1.4.3, though I believe it is the most reliable fix for #7956, which is (rightfully) in the 1.4.2.1 milestone.

@benjamn benjamn added this to the Release 1.4.2.1 milestone Oct 31, 2016
@benjamn
Copy link
Contributor Author

benjamn commented Nov 1, 2016

Many tests were broken by the initial version of this pull request, since we weren't installing babel-runtime in the various test apps used by meteor self-test ..., but that should be fixable.

In light of issues like #7956, I would very much like for app developers
to be responsible for providing node_modules/babel-runtime, and for the
Meteor babel-runtime package to stop attempting to implement a subset of
the helpers.
At this point, the only dependencies installed in this way are
babel-runtime and meteor-node-stubs, but that set of packages will
expand in the future as we move more dependencies to npm.
Though this may seem like a significant change, this package will still
work for older apps as long as the developer follows the instructions
about installing the babel-runtime npm package.

Helps with #7956.
Tests were broken by the new dependency on the babel-runtime npm
package, but fortunately all those tests run `meteor --prepare-app`
whenever an app is created by a self-test, so this change should fix
most of the breakages.
Some tests disable the --prepare-app step, but still need babel-runtime
to be installed.
This is another way to fix the bug I previously fixed by passing
options.allowSyntaxError to optimisticReadJsonOrNull.

To reproduce the bug (and/or verify that it is fixed), run the following
commands in a terminal:

  rm -rf meteor/packages/npm-mongo/.npm
  meteor/meteor --get-ready

Before these two commits, optimisticReadJsonOrNull would throw a
SyntaxError when reading one of the .meteor-portable files because an
asynchronous write to the same file had not yet finished.
This extraction was necessary because importing tools/cli/commands.js is
not entirely side-effect-free, and was interfering with older tests.
This test has been failing intermittently on Circle CI.
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

1 participant