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

Upgrade babel@7 #3785

Merged
merged 11 commits into from
Feb 10, 2018
Merged

Upgrade babel@7 #3785

merged 11 commits into from
Feb 10, 2018

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Jan 31, 2018

First pass at a v7 upgrade for babel. Mostly just dep updates, but it also has some opinions about the default babel config used for a site.

I'd defer a change in the babel plugin api till after this is merged

@ghost ghost assigned jquense Jan 31, 2018
@ghost ghost added the review label Jan 31, 2018
@@ -0,0 +1,167 @@
"use strict";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ARG!

@@ -1,2 +0,0 @@
/*.js
!index.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing stub pacakges because babel now complains if you try and run it without files so these all break the build

Choose a reason for hiding this comment

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

Out of curiosity, what error were you seeing here? This could just be an accidental regression on Babel's side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

babel now exits with a non-zero if the cli call doesn't end up with any files that it can transpile. It's probably not a bad change, but it messes with our scaffolding a bit, which assumes you'll use babel, but these stub packages don't actually have any files in src/

Choose a reason for hiding this comment

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

Gotcha. I don't know if that was an intentional change or not on our end. I feel like as long as the directory itself exists, the fact that there are no files shouldn't be an error.

Choose a reason for hiding this comment

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

}
if (!babelrc.presets) {
babelrc.presets = []
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.

instead of what we were doing if there is no babelrc provide a full default config to use that covers most cases. otherwise let the user pick. no magically borrowing of what gatsby has installed in keeping with the other resolution changes we've been making

Choose a reason for hiding this comment

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

It looks like this may be bugged. babelrc is initialized as

let babelrc = ... || {};

so it should never be falsy?

babelrc.presets = []
babelrc = {
cacheDirectory: true,
plugins: [],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should probably include class properties

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at CRA's babel 7 upgrade PR – just copy their plugin list I think? https://github.com/facebook/create-react-app/pull/3522/files#diff-4dea0a44b1f2f65c2294fd4dae2e25f7R72

@jquense
Copy link
Contributor Author

jquense commented Jan 31, 2018

it should be said that there is no chance netlify will pass because so many deps changed

@KyleAMathews
Copy link
Contributor

Your prediction came true :-)

This is awesome! I'll be going through PRs later & also merging master into v2 so will check this out then.

Copy link

@loganfsmyth loganfsmyth left a comment

Choose a reason for hiding this comment

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

A bit of feedback and questions from my quick glance through.

.babel-preset.js Outdated
Object.assign(
{
loose: true,
debug: !!debug,
useBuiltIns: true,
useBuiltIns: "entry",
shippedProposals: true,
modules: "commonjs",

Choose a reason for hiding this comment

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

This should set sourceType: "unambiguous" probably.

.babel-preset.js Outdated
@@ -25,31 +24,31 @@ function preset(context, options = {}) {
return {

Choose a reason for hiding this comment

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

The filename seems less than ideal. Should we ditch the . prefix on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably, originally this was this was that fake babelrc.js pattern, but we don't actually want it to be a rc file since other packages use it as a preset. I think i left the . simply to keep the files close together, we should probably move this it a real package tho

Choose a reason for hiding this comment

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

Your call really. I get the point as far as keeping them together, I do tend to think dotfiles get confusing though.

@@ -14,11 +14,12 @@
"author": "Kyle Mathews <mathews.kyle@gmail.com>",
"license": "MIT",
"devDependencies": {
"babel-cli": "^6.26.0",
"@babel/cli": "^7.0.0-beta.38",
"@babel/core": "^7.0.0-beta.38",

Choose a reason for hiding this comment

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

I know this was already how it was, but is there any reason to have these each independently depend on @babel/core, when building in the first place depends on the .babel-preset.js from the root? Why not install this in the root instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the preset is in the root but the build call is local to each package. Its a balance between keeping packages reasonablly independant so different folks can maintain them while also making repo wide stuff a bit easier. so since each package depends on babel/cli core is need b/c peer deps

Choose a reason for hiding this comment

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

Gotcha, guess that makes sense then.

@@ -15,18 +15,30 @@ async function onCreateNode({ node, getNode, actions, loadNodeContent }) {
const options = {
sourceType: `module`,

Choose a reason for hiding this comment

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

Probably want sourceType: "unambiguous"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll admit i'm fairly unfamiliar with what this option does

Choose a reason for hiding this comment

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

Until the community starts properly transitioning to using .mjs files, Babel needs to either told what type of files it is going to get, or it needs to be told to guess. Parsing as a module ends up meaning it is auto-strict, among other things. Setting unambiguous basically means it will attempt to autodetect the type, but might get in wrong in some cases. It should avoid the need for strictMode: false though since it'll only be auto-strict if the user used import or export statements.

@@ -15,18 +15,30 @@ async function onCreateNode({ node, getNode, actions, loadNodeContent }) {
const options = {
sourceType: `module`,
allowImportExportEverywhere: true,
strictMode: false,

Choose a reason for hiding this comment

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

This may be addressed by sourceType: "module" here? What was this trying to address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think i copied this whole cloth from the relay-compiler setup, o rather i did in the babel plugin and then copied those options here.

Choose a reason for hiding this comment

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

Fair enough. See my comment above about unambiguous.

"babel-core": "^6.24.1",
"@babel/code-frame": "^7.0.0-beta.38",
"@babel/core": "^7.0.0-beta.38",
"babel-core": "^7.0.0-0",
"babel-loader": "^7.1.2",

Choose a reason for hiding this comment

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

There's a beta version of this that depends on @babel/core directly FYI.

allowImportExportEverywhere: true,
allowReturnOutsideFunction: true,
allowSuperOutsideMethod: true,
sourceType: `module`,

Choose a reason for hiding this comment

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

I'd recommend unambiguous here too.

}
if (!babelrc.presets) {
babelrc.presets = []
babelrc = {

Choose a reason for hiding this comment

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

It looks like this may be bugged. babelrc is initialized as

let babelrc = ... || {};

so it should never be falsy?

@@ -0,0 +1,38 @@
var _require = require(`../index`),

Choose a reason for hiding this comment

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

Looks like this was accidentally committed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, a failure to handle the right babel ignores i think

@@ -1,2 +0,0 @@
/*.js
!index.js

Choose a reason for hiding this comment

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

Out of curiosity, what error were you seeing here? This could just be an accidental regression on Babel's side.

@ghost ghost assigned KyleAMathews Feb 9, 2018
@KyleAMathews KyleAMathews merged commit 4bfe9a9 into v2 Feb 10, 2018
@ghost ghost removed the review label Feb 10, 2018
@KyleAMathews KyleAMathews deleted the babel-upgrade branch February 10, 2018 00:09
@KyleAMathews
Copy link
Contributor

Thanks! Really appreciate your work as always!

mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
* Upgrade babel@7

* Set sourceType to 'unambigious'

* Remove strictMode and use sourceType: unambigious

* Upgrade to babel-loader@8.0.0-beta.0

* remove accidentally commited files + improve .gitignore

* Change order to remove comment ;-)

* Add more stuff

* rm compiled tests folders

* fix some broken tests due to merge

* fix test ignores
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.

3 participants