fix(babel-preset-gatsby-package): remove explicit `@babel/plugin-proposal-class-properties` and let `@babel/preset-env` add it #24640

Js-Brecht · 2020-05-30T10:56:29Z

Description

A constraint was added to @babel/preset-env recently, which is causing babel-preset-gatsby-package to throw the error shown here: Js-Brecht/gatsby-plugin-ts-config#11. (I use babel-preset-gatsby-package for transpiling TS gatsby-* files from gatsby-plugin-ts-config).

Apparently, if loose: true is set for @babel/plugin-proposal-private-methods, then it must also be set to true for @babel/plugin-proposal-class-properties. According to this comment, both @babel/plugin-proposal-private-methods and @babel/plugin-proposal-class-properties are included by @babel/preset-env when shippedProposals: true, and they're assigned uniform options then. However, babel-preset-gatsby-package imports @babel/plugin-proposal-class-properties itself, which overrides that option.

To fix it, I've just added loose: true to @babel/plugin-proposal-class-properties so that it matches @babel/preset-env. Since @babel/preset-env imports those plugins itself, maybe @babel/plugin-proposal-class-properties could just be removed from the list?

pieh · 2020-06-01T17:17:55Z

Since @babel/preset-env imports those plugins itself, maybe @babel/plugin-proposal-class-properties could just be removed from the list?

We have shippedProposals: true in our preset-env settings which make it so preset doesn't add few of plugins (including class properties ones), so I think the change in this PR is actually least invasive one (tho I need to look up meaning of loose setting :) )

That said - I wonder if gatsby-package is right preset we need to look at? Usually this one is used only to build package to publish to npm and shouldn't be causing problem in runtime? Js-Brecht/gatsby-plugin-ts-config#11 is the issue I refer to - unless your plugin make use of that gatsby-package preset - I would expect stuff to be problematic with babel-preset-gatsby one?

---edit

read description more carefully - ok, makes sense that gatsby-package is suspect in this case

Js-Brecht · 2020-06-01T19:29:19Z

We have shippedProposals: true in our preset-env settings which make it so preset doesn't add few of plugins (including class properties ones)

Looks like they use a round-about way to define what plugins the preset pulls:

It uses this list to find what plugin names to pull.
Then it gets the actual imported modules from here.
pluginsFiltered is the plugins list being used.

So it appears as though class-properties and private-methods are being used, but here it sets class-properties to be the same as private-methods. Not sure of the implications of that.

tho I need to look up meaning of loose setting

Yeah, I'm not really sure about that either. If it cannot be loose: true, then maybe private-methods could also be imported, and they could both be set to loose: false, that way it overrides what preset-env is pulling? 🤷‍♂️

Js-Brecht · 2020-06-03T23:15:04Z

So, with class-properties:

loose: true: class members will be defined using an assignment expression.
```
var Foo = function Foo() {
   this.y = 'bar';
}
```

loose: false, class members are defined using Object.defineProperty().

var Foo = function Foo() {
   Object.defineProperty(this, "y", {
      configurable: true,
      enumerable: true,
      writable: true,
      value: 'bar'
   })
}

Seems like the assignment is a little more thorough using loose: false.

In private-methods:

loose: true: private class methods will be defined using Object.defineProperty()
loose: false: private class methods will be defined using a WeakSet().

Here it means the difference between the class accessing private methods with the normal property access methods, or by using .get(). Their example uses some helpers that I haven't looked at, so I don't really know exactly what it's doing without looking deeper. I think the point of the loose property is whether or not private members can be accessed from outside the class.

pieh · 2020-06-03T23:52:50Z

I found this post - https://2ality.com/2015/12/babel6-loose-mode.html helpful when checking on loose - seems like it trades correctness for some perf and potentially some compat with "older engines" (... just not quite sure what this part means in concerete terms - also that blog post was from 2015 so aeons ago in JS time :P)

So it appears as though class-properties and private-methods are being used

I'm not exactly sure if it does (at least with our config) - I was trying this out with some monkey patching in babel (logging config in https://unpkg.com/browse/@babel/core@7.10.2/lib/transform-file.js) to log stuff out and when using babel-preset-gatsby-package version without class-properties (to build one of our packages)- that plugin was not auto added by preset-env.

Js-Brecht · 2020-06-04T00:14:26Z

I think when my plugin is installed it might be winding up with a newer version of @babel/preset-env. If I run yarn upgrade in the Gatsby repository, then try to run a build, I get this error:

Though the "loose" option was set to "true" in your @babel/preset-env config, it will not be used for @babel/plugin-proposal-private-methods since the "loose" mode option was set to "false" for @babel/plugin-proposal-class-properties.
The "loose" option must be the same for @babel/plugin-proposal-class-properties, @babel/plugin-proposal-private-methods and @babel/plugin-proposal-private-property-in-object (when they are enabled): you can silence this warning by explicitly adding
	["@babel/plugin-proposal-private-methods", { "loose": false }]
to the "plugins" section of your Babel config.

Js-Brecht · 2020-06-04T00:17:19Z

But if I don't do any upgrade, then it works fine.

I totally understand if you don't want to take a chance with this right now. I can also adjust the options of class-properties during runtime; I'm already doing it once for tranform-runtime to inject the absoluteRuntime (for yarn2 support)

pieh · 2020-06-04T10:07:19Z

I think when my plugin is installed it might be winding up with a newer version of @babel/preset-env. If I run yarn upgrade in the Gatsby repository, then try to run a build, I get this error:

We recently bumped @babel/* packages and we do get those now too in here - but it seems like those are warnings and not errors (as in it's not terminal)

I totally understand if you don't want to take a chance with this right now. I can also adjust the options of class-properties during runtime; I'm already doing it once for tranform-runtime to inject the absoluteRuntime (for yarn2 support)

We do need to address it in some way, with our recent babel/* bump in monorepo this puts more priority on this

wardpeet · 2020-06-04T10:25:36Z

I feel we should exclude the transforms from babel-preset-env so we use ours and not the one from preset-env, this would remove any issues we will have in the future.

I'm currently already doing it for the core-js refactor I'm doing

pieh · 2020-06-04T10:59:06Z

I'm not exactly sure if it does (at least with our config) - I was trying this out with some monkey patching in babel (logging config in https://unpkg.com/browse/@babel/core@7.10.2/lib/transform-file.js) to log stuff out and when using babel-preset-gatsby-package version without class-properties (to build one of our packages)- that plugin was not auto added by preset-env.

After trying it again from scratch after chat with @wardpeet it seems like my previous comment is wrong. I can't really reproduce problem I had anymore and I also don't exactly remember errors I was seeing :S

pieh · 2020-06-04T11:15:51Z

After trying it again from scratch after chat with @wardpeet it seems like my previous comment is wrong. I can't really reproduce problem I had anymore and I also don't exactly remember errors I was seeing :S

Lol - sorry for back and forth - I figured out how I was getting those errors before - it was before #24432 (bump for @babel/*) was merged and then errors - after commenting out explicit class-properties plugin I was seeing things like:

➜  gatsby-remark-katex git:(40d241b12e) ✗ yarn prepare              
yarn run v1.21.0
$ cross-env NODE_ENV=production npm run build
npm WARN lifecycle The node binary used for scripts is /var/folders/7b/7_tqb8f50bx7jhyj0bn9r8v80000gn/T/yarn--1591268500771-0.2962234383578015/node but npm is using /Users/misiek/.nvm/versions/node/v10.16.3/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.

> gatsby-remark-katex@3.3.2 build /Users/misiek/dev/gatsby/packages/gatsby-remark-katex
> babel src --out-dir . --ignore "**/__tests__"

@babel/preset-env: `DEBUG` option

Using targets:
{
  "node": "10.13"
}

Using modules transform: commonjs

Using plugins:
  proposal-numeric-separator { "node":"10.13" }
  proposal-nullish-coalescing-operator { "node":"10.13" }
  proposal-optional-chaining { "node":"10.13" }
  syntax-json-strings { "node":"10.13" }
  syntax-optional-catch-binding { "node":"10.13" }
  syntax-async-generators { "node":"10.13" }
  syntax-object-rest-spread { "node":"10.13" }
  transform-modules-commonjs { "node":"10.13" }
  proposal-dynamic-import { "node":"10.13" }

Using polyfills with `entry` option:
{ SyntaxError: /Users/misiek/dev/gatsby/packages/gatsby-remark-katex/src/index.js: Support for the experimental syntax 'classProperties' isn't currently enabled (7:20):

   5 | class Bork {
   6 |   //Property initializer syntax
>  7 |   instanceProperty = `bork`
     |                    ^
   8 |   boundFunction = () => {
   9 |     return this.instanceProperty
  10 |   }

Add @babel/plugin-proposal-class-properties (https://git.io/vb4SL) to the 'plugins' section of your Babel config to enable transformation.
If you want to leave it as-is, add @babel/plugin-syntax-class-properties (https://git.io/vb4yQ) to the 'plugins' section to enable parsing.
    at Object._raise (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:742:17)
    at Object.raiseWithData (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:735:17)
    at Object.expectPlugin (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:8784:18)
    at Object.parseClassProperty (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:12133:12)
    at Object.parseClassProperty (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:2947:18)
    at Object.pushClassProperty (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:12093:30)
    at Object.parseClassMemberWithIsStatic (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:12026:14)
    at Object.parseClassMember (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:11963:10)
    at Object.parseClassMember (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:2837:11)
    at withTopicForbiddingContext (/Users/misiek/dev/gatsby/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:11908:14)
  loc: Position { line: 7, column: 19 },
  pos: 180,
  missingPlugin: [ 'classProperties' ],
  code: 'BABEL_PARSE_ERROR' }
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! gatsby-remark-katex@3.3.2 build: `babel src --out-dir . --ignore "**/__tests__"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the gatsby-remark-katex@3.3.2 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/misiek/.npm/_logs/2020-06-04T11_01_44_240Z-debug.log
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

But this is not happening after the bump - so I think we can go ahead with your suggestion to remove explicit class-properties from plugin list (and likely direct dependencies of that preset?) and let preset-env handle this (which should also ensure loose setting is consistent there)

---edited:
For reference - after the bump output looks like this:

➜  gatsby-remark-katex git:(master) ✗ yarn prepare
yarn run v1.21.0
$ cross-env NODE_ENV=production npm run build
npm WARN lifecycle The node binary used for scripts is /var/folders/7b/7_tqb8f50bx7jhyj0bn9r8v80000gn/T/yarn--1591269431100-0.9638090414665175/node but npm is using /Users/misiek/.nvm/versions/node/v10.16.3/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.

> gatsby-remark-katex@3.3.3 build /Users/misiek/dev/gatsby/packages/gatsby-remark-katex
> babel src --out-dir . --ignore "**/__tests__"

@babel/preset-env: `DEBUG` option

Using targets:
{
  "node": "10.13"
}

Using modules transform: commonjs

Using plugins:
  proposal-numeric-separator { "node":"10.13" }
  proposal-class-properties { "node":"10.13" }
  proposal-private-methods { "node":"10.13" }
  proposal-nullish-coalescing-operator { "node":"10.13" }
  proposal-optional-chaining { "node":"10.13" }
  syntax-json-strings { "node":"10.13" }
  syntax-optional-catch-binding { "node":"10.13" }
  syntax-async-generators { "node":"10.13" }
  syntax-object-rest-spread { "node":"10.13" }
  transform-modules-commonjs { "node":"10.13" }
  proposal-dynamic-import { "node":"10.13" }

Using polyfills with `entry` option:

[/Users/misiek/dev/gatsby/packages/gatsby-remark-katex/src/index.js] Import of @babel/polyfill was not found.
Successfully compiled 1 file with Babel (1889ms).
✨  Done in 2.99s.

(it auto adds class-properties)

So I guess my confusion was mostly related to some changes happening to babel packages before and after the babel/* bump

Js-Brecht · 2020-06-04T16:14:26Z

We recently bumped @babel/* packages and we do get those now too in here - but it seems like those are warnings and not errors (as in it's not terminal)

I'm not sure exactly why it's throwing an error on mine; it's the same message, though. The major difference is that mine is doing runtime transpiling through @babel/register, and building Gatsby packages is done on the CLI... ? I don't really know if that's why it throws instead, but it seems reasonable to assume.

so I think we can go ahead with your suggestion to remove explicit class-properties from plugin list (and likely direct dependencies of that preset?) and let preset-env handle this (which should also ensure loose setting is consistent there)

I just want to verify: you want to remove class-properties from preset-gatsby-package? I ask because it sounded like @wardpeet wanted the config to be more explicit, I assume by adding private-methods as well? Unless there's another way to tell preset-env not to include class-properties or private-methods.

pieh · 2020-06-04T19:05:15Z

I just want to verify: you want to remove class-properties from preset-gatsby-package? I ask because it sounded like @wardpeet wanted the config to be more explicit, I assume by adding private-methods as well? Unless there's another way to tell preset-env not to include class-properties or private-methods.

Ops, sorry for spreading confusion here - I didn't realize @wardpeet left comments on the PR that didn't align with my suggestion :P. As mentioned in previous comments - I did chat with him about this PR / issue with loose and here's part of "transcript" (just so the exchange is not lost)

pieh:  12:47
but it seems like removing explicit class-properties and let preset-env handle this is way to go - just need to verify it a bit more

Ward:  12:49 PM
or the other way around

pieh:  12:49 PM
sure, yeah

Ward:  12:49 PM
probably letting babel-preset-env handle it is better cause it's only used when necessary

this was couple of minutes after Ward's comment in the PR now that I check times (CEST in my chat logs) - so I think we should be ok with letting preset-env handle autoadding proposal-class-properties as needed

TBH, I don't have any issue with doing this other way around - keeping explicit proposal-class-properties with loose: true and excluding it from being auto added by @babel/preset-env (this is mostly to not have duplicate plugins in there because AFAIK babel does not dedupe plugin instances and duplicates just "waste" CPU cycles

Js-Brecht · 2020-06-05T00:19:21Z

Okay, I went ahead and removed @babel/plugin-proposal-class-properties from the plugin list

Js-Brecht · 2020-06-05T00:50:20Z

That results in this error:

Support for the experimental syntax 'classProperties' isn't currently enabled

…-package

pieh · 2020-06-05T07:33:17Z

That results in this error:

Support for the experimental syntax 'classProperties' isn't currently enabled

I think that's because this branch was branched of master before we bumped babel/* (mostly just yarn.lock bump is important here, because version selectors already would pull newest if not for a lock file). So my comment in #24640 (comment) probably apply here.

I just merged master in here to validate my assumption

pieh · 2020-06-05T08:17:54Z

Ok, so merging master in fixed the problem. I'm good with the PR as it is right now - I just want to make sure this also fixes original problem described in gatsby-plugin-ts-config as well - can you let me know if it does or we need to iterate further?

Js-Brecht · 2020-06-05T16:39:05Z

I monkey patched gatsby-package to exclude class-properties, and it works.

Versions being used:

babel-preset-gatsby-package@0.4.3
@babel/preset-env@7.10.2

Question: should @babel/plugin-proposal-class-properties be removed from the dependencies for babel-preset-gatsby-package?

pieh · 2020-06-08T07:50:59Z

Question: should @babel/plugin-proposal-class-properties be removed from the dependencies for babel-preset-gatsby-package?

I think so yes. @babel/preset-env has @babel/plugin-proposal-class-properties as its own dependency ( https://unpkg.com/browse/@babel/preset-env@7.10.2/package.json )

Js-Brecht · 2020-06-08T16:13:09Z

Done

pieh · 2020-06-09T08:40:33Z

Will do one last master merge and get this merged and published

…-package

pieh

Thanks @Js-Brecht!

pieh · 2020-06-09T09:32:39Z

Published in babel-preset-gatsby-package@0.4.4

Js-Brecht · 2020-06-09T16:08:29Z

thanks!

Fixes loose mode issues with Babel gatsbyjs/gatsby#24640 babel/babel#11622

…osal-class-properties` and let `@babel/preset-env` add it (gatsbyjs#24640) * fix error from babel about `loose: true` * update snapshots for tests * remove @babel/plugin-proposal-class-properties * update jest snapshots * remove class-properties from dependencies Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>

fix error from babel about loose: true

2685589

Js-Brecht requested a review from a team as a code owner May 30, 2020 10:56

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 30, 2020

Js-Brecht mentioned this pull request May 30, 2020

'loose' mode configuration must be the same for both @babel/plugin-proposal-class-properties and @babel/plugin-proposal-private-methods Js-Brecht/gatsby-plugin-ts-config#11

Closed

Js-Brecht changed the title ~~fix error from babel about loose: true~~ fix(babel-preset-gatsby-package) adjust loose mode configuration to comply with babel constraint May 30, 2020

update snapshots for tests

7b9a721

pieh added status: needs core review Currently awaiting review from Core team member topic: webpack/babel Webpack or babel and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 1, 2020

pieh self-assigned this Jun 4, 2020

remove @babel/plugin-proposal-class-properties

edf07b3

update jest snapshots

2a15fea

Merge remote-tracking branch 'upstream/master' into fix-preset-gatsby…

58f67a6

…-package

pieh changed the title ~~fix(babel-preset-gatsby-package) adjust loose mode configuration to comply with babel constraint~~ fix(babel-preset-gatsby-package): remove explicit @babel/plugin-proposal-class-properties and let @babel/preset-env add it Jun 5, 2020

remove class-properties from dependencies

10e13a9

Merge remote-tracking branch 'upstream/master' into fix-preset-gatsby…

54252e7

…-package

pieh approved these changes Jun 9, 2020

View reviewed changes

pieh merged commit 272ba4f into gatsbyjs:master Jun 9, 2020

Js-Brecht deleted the fix-preset-gatsby-package branch June 9, 2020 16:07

baerrach added a commit to baerrach/gatsby-remark-plantuml that referenced this pull request Jul 31, 2020

chore(deps): upgrade babel-preset-gatsby-package to 0.4.4

65fc202

Fixes loose mode issues with Babel gatsbyjs/gatsby#24640 babel/babel#11622

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(babel-preset-gatsby-package): remove explicit `@babel/plugin-proposal-class-properties` and let `@babel/preset-env` add it #24640

fix(babel-preset-gatsby-package): remove explicit `@babel/plugin-proposal-class-properties` and let `@babel/preset-env` add it #24640

Js-Brecht commented May 30, 2020

pieh commented Jun 1, 2020 •

edited

Js-Brecht commented Jun 1, 2020 •

edited

Js-Brecht commented Jun 3, 2020 •

edited

pieh commented Jun 3, 2020

Js-Brecht commented Jun 4, 2020 •

edited

Js-Brecht commented Jun 4, 2020 •

edited

pieh commented Jun 4, 2020 •

edited

wardpeet commented Jun 4, 2020 •

edited

pieh commented Jun 4, 2020

pieh commented Jun 4, 2020 •

edited

Js-Brecht commented Jun 4, 2020

pieh commented Jun 4, 2020 •

edited

Js-Brecht commented Jun 5, 2020

Js-Brecht commented Jun 5, 2020

pieh commented Jun 5, 2020

pieh commented Jun 5, 2020

Js-Brecht commented Jun 5, 2020

pieh commented Jun 8, 2020

Js-Brecht commented Jun 8, 2020

pieh commented Jun 9, 2020

pieh left a comment

pieh commented Jun 9, 2020

Js-Brecht commented Jun 9, 2020

fix(babel-preset-gatsby-package): remove explicit @babel/plugin-proposal-class-properties and let @babel/preset-env add it #24640

fix(babel-preset-gatsby-package): remove explicit @babel/plugin-proposal-class-properties and let @babel/preset-env add it #24640

Conversation

Js-Brecht commented May 30, 2020

Description

pieh commented Jun 1, 2020 • edited

Js-Brecht commented Jun 1, 2020 • edited

Js-Brecht commented Jun 3, 2020 • edited

pieh commented Jun 3, 2020

Js-Brecht commented Jun 4, 2020 • edited

Js-Brecht commented Jun 4, 2020 • edited

pieh commented Jun 4, 2020 • edited

wardpeet commented Jun 4, 2020 • edited

pieh commented Jun 4, 2020

pieh commented Jun 4, 2020 • edited

Js-Brecht commented Jun 4, 2020

pieh commented Jun 4, 2020 • edited

Js-Brecht commented Jun 5, 2020

Js-Brecht commented Jun 5, 2020

pieh commented Jun 5, 2020

pieh commented Jun 5, 2020

Js-Brecht commented Jun 5, 2020

pieh commented Jun 8, 2020

Js-Brecht commented Jun 8, 2020

pieh commented Jun 9, 2020

pieh left a comment

Choose a reason for hiding this comment

pieh commented Jun 9, 2020

Js-Brecht commented Jun 9, 2020

fix(babel-preset-gatsby-package): remove explicit `@babel/plugin-proposal-class-properties` and let `@babel/preset-env` add it #24640

fix(babel-preset-gatsby-package): remove explicit `@babel/plugin-proposal-class-properties` and let `@babel/preset-env` add it #24640

pieh commented Jun 1, 2020 •

edited

Js-Brecht commented Jun 1, 2020 •

edited

Js-Brecht commented Jun 3, 2020 •

edited

Js-Brecht commented Jun 4, 2020 •

edited

Js-Brecht commented Jun 4, 2020 •

edited

pieh commented Jun 4, 2020 •

edited

wardpeet commented Jun 4, 2020 •

edited

pieh commented Jun 4, 2020 •

edited

pieh commented Jun 4, 2020 •

edited