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

fix: babel transform-runtime bug #69

Closed
wants to merge 4 commits into from

Conversation

cfnelson
Copy link
Contributor

@cfnelson cfnelson commented Jul 7, 2017

This PR has been created to resolve a babel-runtime bug. For more details see here #68

Changes made:

  • Changed scripts prepublish to prepare in package.json
  • Added engines to package.json for node & npm.
  • Removed un-necessary or unused babel plugins
  • Switched to using babel-preset-env & setting target to "node: "current" "node: ""4.8.3"`.
  • prepublish.sh script no longer uses "transform-runtime"
  • removed dist folder from .gitignore so it can be tested against without npm link. Please see below in Testing.

A PR we should keep our eyes on for specifying engines in babel-preset-env. babel/babel-preset-env#114

Testing:

You can test locally by running the below command in any project that is using the merge-graphql-schemas package.

npm install -S git://github.com/okgrow/merge-graphql-schemas#fix-babel-transform-runtime.

Additionally the branch was tested via npm link against a node app running node 8.0.0/npm 5.0.3 & meteor npm link against a meteor app running node 4.8.3/npm 4.6.1.

I would like others to further test this as I have seen some odd behaviour with npm link. Remember to rm -rf node_modules.

@cfnelson cfnelson changed the title fix + refactor: remove unused babel deps, fix .babelrc & prepublish.sh fix: babel transform-runtime bug Jul 7, 2017
Copy link
Contributor

@xavxyz xavxyz left a comment

Choose a reason for hiding this comment

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

Less code is better 😎

@@ -8,8 +8,12 @@
"url": "https://github.com/okgrow/merge-graphql-schemas.git"
},
"license": "MIT",
"engines": {
"node": ">=4.8.3",
"npm": ">=4.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need npm >= 4 ? 🤔

Copy link
Contributor Author

@cfnelson cfnelson Jul 7, 2017

Choose a reason for hiding this comment

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

@xavcz Meteor is 4.8.3 , will drop it once meteor 1.6 is released.

"scripts": {
"prepublish": ". ./scripts/prepublish.sh",
"prepare": ". ./scripts/prepublish.sh",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is running npm publish triggering this command normally?

Copy link
Contributor Author

@cfnelson cfnelson Jul 7, 2017

Choose a reason for hiding this comment

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

@xavcz yes, npm publish & npm install will trigger the pre-publish in npm 4 land. In npm 5 pre-publish only runs on publish. See these two threads for more details npm/npm#16685 & npm/npm#10074 (comment)

@aarohmankad
Copy link

Any eta on this PR getting into the production version of the module?

@cfnelson cfnelson mentioned this pull request Jul 21, 2017
@RodMachado
Copy link
Contributor

Changes done in PR #71. Closing this one.

@RodMachado RodMachado closed this Jul 28, 2017
@cfnelson cfnelson deleted the fix-babel-transform-runtime branch September 15, 2017 21:03
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.

4 participants