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

feat: update babel to 7 #67

Merged
merged 50 commits into from
Jul 4, 2022
Merged

feat: update babel to 7 #67

merged 50 commits into from
Jul 4, 2022

Conversation

aminya
Copy link
Member

@aminya aminya commented Jul 26, 2020

Copied from atom#21623:

Description of the Change

This upgrades babel 5 to babel 7. It targets Electron 11, so we now run modern JavaScript! 🎉

This PR is the result of two years of effort and trying.
I had to fix electron-link so:

I developed two plugins specifically for Atom to support its special situation:

  • babel-preset-atomic this is the plugin that includes all the other babel plugins. It has babel-preset (configured for Electron 6), babel-flow, babel-react, and many of the proposal plugins that are equivalents of the similar plugins included in the deprecated stage-0 package (e.g., class properties, private properties, decorators, private methods, etc.). It adds some of the new proposal plugins, such as optional chaining. It also allows us to do compile-time calculations using codegen and prevel.

See the full list of plugins that babel-preset-atomic has in this package.json.

  • babel-plugin-transform-not-strict: this removes 'use strict' whenever 'use babel' (or similar triggers) are used. This is because Babel 6/7 now assumes all the ES modules (those who use import/export) are strict by default. However, this breaks some of the Atom packages because they do non-strict things in ES modules. So, I developed this plugin to remove 'use strict' when the triggers are used. I have baked a feature in this plugin that allows removing 'use strict' selectively by adding 'not strict' directive. Ultimately, we should add this 'not strict' to any file that uses JS loosely in ES modules. An example of this is spec/update-process-env-spec.js

There are two move plugins included for backward compatibility:

  • babel-add-module-exports: this plugin circumvents the fact that ES modules export default by "default" keyword. This means we need to call require().default to consume them. This plugin works in combination with transform-not-strict to add module.exports= to the packages that export default things that should have been named exports. For the packages' root, I already made another PR that adds support for both cases, but we cannot patch require for the packages' internal requires as we only have access to the root.

  • @babel/plugin-transform-reserved-words that allows the uses of the old banned keywords such as package or debugger.

Fixed the loose usage of JavaScript in the following packages:

Benefits

This PR has several benefits:

  • Allows building Atom with Node 12.16 or above (see this issue). This will be crucial for upgrading Electron.
  • This allows using the new babel plugins (e.g., optional chaining) just by including "use babel".
  • This also enhances the performance of babel compiling.

Previous attempts

#67
atom#20965

Release Notes

  • Updates babel to babel 7 🎉

@DeeDeeG
Copy link
Member

DeeDeeG commented Jul 26, 2020

If I understand it correctly, this makes newer versions of Node able to build Atom; I think babel 7 has fixes for use with more-recent Node versions.

atom#21091

Affected versions of Node:

  • 12.17.0 and newer (can use 12.16.x and older)
  • 13.2.0 and newer (can use 13.0.0? I think.)
  • 14.0.0 and newer (all versions 14.0.0 and newer are affected).

@aminya
Copy link
Member Author

aminya commented Jul 26, 2020

Yes. Check my comment here: atom#21091 (comment)

@aminya
Copy link
Member Author

aminya commented Jul 26, 2020

I did not notice that we have babel-spec too 😄. I guess I need to update that as well.

@aminya
Copy link
Member Author

aminya commented Jul 26, 2020

Async to generator plugin is not working nicely. I am going to remove it. I also will use our atomic babel preset instead of this whole thing. That is easier to maintain.

@aminya
Copy link
Member Author

aminya commented Jul 26, 2020

NPM's git installer acts weird. Why it can't install some of the packages?!

@DeeDeeG
Copy link
Member

DeeDeeG commented Jul 26, 2020

FWIW, I believe it clones the git dependency, tries to install the dependency in a temp folder (if there is a package-lock.json checked into the repo of the git dependency, this is used! Not sure why.) and then... throws out everything from the temp folder and just uses the package.json from the git dependency?? Or something? It's a very confusing process.

And as far as I can tell, this causes bizarre errors unique to the CI environment. I can't reproduce the failures locally e.g. when setting apm to be a git dependency.

Have to wonder why it's this complicated for git dependencies, but I supposethe publishing process usually strips oackage-lock.json and similar files... Which isn't the case in a bare git repo? But I would assume package-lock.json would be ingored... 🤷

@DeeDeeG
Copy link
Member

DeeDeeG commented Jul 27, 2020

Not sure if this will help, but try with npm >= 6.14.2. Their dependency hosted-git-info was bumped a few times with various fixes.

https://github.com/npm/cli/blob/latest/CHANGELOG.md#6142-2020-03-03

@aminya
Copy link
Member Author

aminya commented Jul 27, 2020

This line is causing issues:
https://github.com/atom/atom/blob/4a9d56f52cab00a22744858d4077600d5365197a/packages/incompatible-packages/lib/view-uri.js#L3

I can fix this. We just need to name export it or remove that @babel

@aminya aminya force-pushed the babel7 branch 2 times, most recently from 1062c89 to 4b1334c Compare July 30, 2020 01:34
@aminya
Copy link
Member Author

aminya commented Jul 30, 2020

Oh wow! Most of the tests have passed! 🎉 The problem was addDefaultProperty option which now I set it to false. We may bring it back later once I updated babel-plugin-add-module-exports in atom-community/babel-plugin-add-module-exports#4

I just need to fix the rest:

##[error] The 'core-render-process for D:/a/1/s/spec/babel-spec.coffee.' test step finished with a non-zero exit code 
 You can run the test again using: 
	 D:/a/1/s/out/Atom Dev x64/atom-dev.exe --resource-path D:\a\1\s --test D:/a/1/s/spec/babel-spec.coffee
##[error] The 'core-render-process for D:/a/1/s/spec/compile-cache-spec.coffee.' test step finished with a non-zero exit code 
 You can run the test again using: 

These tests should actually be updated themselves.

@aminya
Copy link
Member Author

aminya commented Jul 31, 2020

So the problem is that the bundled code cannot find the babel plugins. I am not sure what is happening inside assar when electron-link bundles it. I will switch to babel-preset-atomic and will require it explicitly.

Previously babel-core had all the deps Atom needed for transpiling. But know babel uses separate plugins. So using babel-preset-atomic we can get the same effect.

@aminya
Copy link
Member Author

aminya commented Jul 31, 2020

Including babel dependencies in the snapshot fails. I wonder if we can declare the babel dependencies as external.

@aminya
Copy link
Member Author

aminya commented Aug 17, 2020

I tried 4 different bundlers for bundling babel: https://github.com/atom-ide-community/babel-preset-atomic/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc

All of them fail. We need to find a way to declare babel as external, but let Atom call it during normal loading.

@aminya aminya added the help wanted Extra attention is needed label Aug 17, 2020
New enhancements automation moved this from Done to In progress Jul 22, 2021
Runtime Performance automation moved this from Done to In progress Jul 22, 2021
@icecream17 icecream17 mentioned this pull request Feb 24, 2022
4 tasks
@aminya aminya changed the title update babel to 7 feat: update babel to 7 Jul 2, 2022
@aminya

This comment was marked as resolved.

@aminya
Copy link
Member Author

aminya commented Jul 2, 2022

This basically works fine now! 🎉

@aminya
Copy link
Member Author

aminya commented Jul 3, 2022

It seems that the find-and-replace package needs some updating.

Copy link
Member Author

@aminya aminya left a comment

Choose a reason for hiding this comment

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

This is ready! 🎉 I am going to merge this tomorrow.

@aminya aminya merged commit 07fd453 into master Jul 4, 2022
New enhancements automation moved this from In progress to Done Jul 4, 2022
Runtime Performance automation moved this from In progress to Done Jul 4, 2022
@aminya aminya deleted the babel7 branch July 4, 2022 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants