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

Update Babel to Current Standard "babel-preset-env" #4812

Merged
merged 6 commits into from
Jul 24, 2018

Conversation

ajnauleau
Copy link
Contributor

@ajnauleau ajnauleau commented Jul 16, 2018

Install babel dependencies by running

npm install babel-cli -D
npm install babel-preset-env -D

updated .babelrc file inside your project and add the following code inside it:

{
  "presets": ["env"]
}

Update package.json and add same code within, while keeping current settings.

Change the order of presets to "env", "react", "stage-0".

Build, import into extensions, and tested.

Fixed.

@bdresser
Copy link
Contributor

@whymarrh or @alextsg could you take a look when you get a sec

@whymarrh whymarrh self-assigned this Jul 17, 2018
@whymarrh whymarrh self-requested a review July 17, 2018 20:28
@whymarrh
Copy link
Contributor

@ajnauleau thanks! I'll take a look at this in a bit.

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

Two small things and can we make sure that we regenerate the package-lock.json from npm@6 on Node 8

package.json Outdated
@@ -54,10 +54,17 @@
"babelify",
{
"presets": [
"es2015",
"stage-0"
["env", {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might be better off removing the options here and having it default to using what's in the .babelrc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like Babelify doesn't recognise .babelrc anymore with babel@6, so there wouldn't actually be any need to update .babelrc, but the package.json file is still necessary. I think we still should keep .babelrc updated, incase it could have other uses in the future.

babel/babelify#151

babel/babelify#151

package.json Outdated
"babel-preset-react": "^6.24.1",
"babel-preset-stage-0": "^6.24.1",
"babel-register": "^6.7.2",
"babelify": "^8.0.0",
"beefy": "^2.1.5",
"brfs": "^1.6.1",
"browserify": "^16.1.1",
"browserify": "^16.2.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop the changes to non-babel packages? (We could move them to a different PR if we want to bump them independently?)

Copy link
Contributor

Choose a reason for hiding this comment

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

See also: ganache-core and node-sass below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@whymarrh - Solved as well, looks like the PR is good to merge.

@ajnauleau
Copy link
Contributor Author

Gulp build, and testing on chrome worked on my local environment, but once pushed to PR didn't pass circleCI. Removed commit regenerating package-lock.json and pushed to PR, checks out now. Any suggestions on regenerating package-lock.json file?

@ajnauleau
Copy link
Contributor Author

Bugs seems to have been fixed, worth reviewing now @whymarrh

@ajnauleau
Copy link
Contributor Author

Circle-CI isn't running npm install before building, missing node_modules -- build works locally, will keep looking into this.

@brunobar79
Copy link
Contributor

brunobar79 commented Jul 21, 2018

@ajnauleau make sure you are using node 8.11.3 and npm 6.1.0. If you are using nvm, you can type nvm use and it will switch automatically to the right version.

Once you do that, nuke your node_modules folder, run npm install and commit / push the updated package-lock.json

That should fix the issue. I’ll update the docs later today.

@ajnauleau
Copy link
Contributor Author

@brunobar79 updated node and npm with nvm but I don't think that was the issue. The tests aren't running npm install before running , therefore the modules such as gulp are missing.

Scratching my head on this one...

@brunobar79
Copy link
Contributor

brunobar79 commented Jul 21, 2018

@ajnauleau npm install IS running (see here) but there's a diff generated on the package-lock.json after running npm install on the CI server that makes the build to restore the wrong cache key

That's usually because the package-lock.json is not updated or the node/npm version is incorrect.
Anyway, this issue became very annoying for a lot of people so I just opened #4847 which should take care of the issue.

I'll ping you here once that makes it to develop and you can pull develop, re-run npm i, commit and push and I'm sure the build will pass.

Thanks for your PR and sorry for dealing with this!

@ajnauleau
Copy link
Contributor Author

@brunobar79 after reviewing the tests, it seems like circle-ci was reverting to a cached version on the server. Your fix in #4847 seems to resolve it-- will wait until your update, thanks.

@brunobar79
Copy link
Contributor

@ajnauleau my PR just got merged.

ajnauleau added a commit to ajnauleau/metamask-extension that referenced this pull request Jul 22, 2018
@ajnauleau
Copy link
Contributor Author

@whymarrh - I'm unable to review the requested changes you made, would you be able to check this out? Seems all good to go.

Thanks!

@whymarrh
Copy link
Contributor

Awesome, thanks @ajnauleau! This LGTM and I don't think it increases our (already slow) build time.

I can confirm that the dist builds (npm run dist) are still being browserified (refs #3628 and #3657).

@whymarrh whymarrh merged commit 653e42c into MetaMask:develop Jul 24, 2018
@ajnauleau ajnauleau deleted the babel-trans-env branch July 24, 2018 23:48
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