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.

Sign up for GitHub

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

Jump to bottom

for #950, update babel to v7 #1077

Merged
merged 6 commits into from
Jun 5, 2019
Merged

for #950, update babel to v7 #1077

merged 6 commits into from
Jun 5, 2019

Conversation

catarak
Copy link
Member

@catarak catarak commented May 16, 2019

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)
  • is descriptively named and links to an issue number, i.e. Fixes #123

I upgraded babel to v7, but since I changed a lot of core things I think this needs a review!

@catarak catarak requested a review from andrewn May 16, 2019 21:00
andrewn
andrewn reviewed May 21, 2019
View reviewed changes
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
andrewn
andrewn reviewed May 21, 2019
View reviewed changes
webpack/config.dev.js Outdated Show resolved Hide resolved
.babelrc Show resolved Hide resolved
.babelrc
"transform-react-inline-elements"
"@babel/plugin-transform-react-constant-elements",
"@babel/plugin-transform-react-inline-elements",
"@babel/plugin-syntax-dynamic-import",
Copy link
Member

Choose a reason for hiding this comment

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

The @babel/plugin-syntax-dynamic-import documentation says:

Currently, @babel/preset-env is unaware that using import() with Webpack relies on Promise internally. Environments which do not have builtin support for Promise, like Internet Explorer, will require both the promise and iterator polyfills be added manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

i think this mostly affects IE support. to get this to work, does core-js need to be added as a dependency? i fell into a hole reading about @babel/preset-env with useBuiltIns: "usage", but can't figure out whether using this feature affects this plugin.

Copy link
Member

@andrewn andrewn May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I think setting useBuiltIns to anything other than false will insert core-js imports for the required polyfills.

core-js seems to already be included in node_modules because of the following dependencies:

@babel/preset-env ▶️ core-js-compat ▶️ core-js

So this should just work as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, i just pushed a change to the webpack configs to support this.

.babelrc Show resolved Hide resolved
@andrewn
Copy link
Member

andrewn commented May 21, 2019 •
edited
Loading

Wow, thanks for tackling this!

I've added some comments above based on each plugin's documentation.

I've also done some manual testing and things seem to work find so perhaps the plugin comments above aren't relevant?

Manual test plan

What I did:

  • Run npm ci to ensure clean install of modules
  • Run npm start to test dev mode
I performed the following manual actions
  • Open site
  • Log in
  • Edit sketch
  • Turn on Auto refresh
  • Play/Stop sketch
  • Edit index.html
  • Edit styles.css
  • Rename styles.css -> index.css
  • Add folder folder
  • Add file 'something.js' to folder with console.log('added');
  • Include folder/something.js to index.html
  • File -> Examples
  • Open Hello P5: Weather
  • Open Settings
  • Change font size
  • Change theme to Dark
  • Enable "Accessible text based canvas" -> Sound
  • Open Keyboard Shortcuts modal
  • Comment line via keyboard shortcut (Cmd + /)
  • Find "position" in editor (Cmd + F)
  • File -> Download (check sketch zip opens)
  • Sketch -> Add File (test file upload)
  • Display in sketch via index.html

Browsers:

  • Firefox 66.0.5 (64-bit) macOS Mojave 10.14.4
  • Safari 12.1 (14607.1.40.1.4) macOS Mojave 10.14.4

Shall I also test in Microsoft Edge? What versions does the Editor support?

@catarak
Copy link
Member Author

catarak commented May 21, 2019

Shall I also test in Microsoft Edge? What versions does the Editor support?

There have been some issues with Edge that I've never been able to figure out/fix (see #27), but as far as I know, Edge should be supported. I have a free BrowserStack plan because this project is open source, should I invite you?

@andrewn
Copy link
Member

andrewn commented May 22, 2019

I have a free BrowserStack plan because this project is open source, should I invite you?

@catarak Yes please!

@catarak
Copy link
Member Author

catarak commented May 22, 2019

when i ran the command npx babel-upgrade, it only upgraded babel to 7.0.0—do you think i should try to update it to 7.4?

@andrewn
Copy link
Member

andrewn commented May 23, 2019

I think if we're going to all the effort of testing the changes, it's worth upgrading to the latest released version. 🚀

@catarak
Copy link
Member Author

catarak commented May 23, 2019

cool, i'm going to upgrade everything to LTS

@andrewn
Copy link
Member

andrewn commented Jun 4, 2019

I just tested the LTS babel in Microsoft Edge 44.17763.1.0 and it's looking good to me!

@catarak
Copy link
Member Author

catarak commented Jun 5, 2019

okay great. i'm going to merge this!

@catarak catarak merged commit 18f646b into master Jun 5, 2019
catarak added a commit that referenced this pull request Jun 11, 2019
@catarak
for #950, update babel to v7 (#1077)
6f1b6fd 
* for #950, upgrade babel to v7

* fix linting errors

* for #950, remove @babel/core from devDependencies (so it's only in dependencies) and change babel-loader config to use .babelrc

* for #950, changes to .babelrc to make  work

* for #950, include core-js modules in webpack config for IE support with babel/plugin-syntax-dynamic-import

* for #950, update babel and associated packages to LTS
@catarak catarak deleted the update-babel branch June 21, 2019 18:56
catarak added a commit that referenced this pull request Jul 22, 2019
@catarak
for #950, update babel to v7 (#1077)
e2676ec 
* for #950, upgrade babel to v7

* fix linting errors

* for #950, remove @babel/core from devDependencies (so it's only in dependencies) and change babel-loader config to use .babelrc

* for #950, changes to .babelrc to make  work

* for #950, include core-js modules in webpack config for IE support with babel/plugin-syntax-dynamic-import

* for #950, update babel and associated packages to LTS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewn andrewn andrewn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@catarak @andrewn