Pin @babel/plugin-transform-classes to v7.0.0-beta.35 #216

bicknellr · 2018-04-24T21:48:45Z

babel/babel#7020 (included in v7.0.0-beta.36) breaks our assumptions about how babel will transform ES6 classes. Particularly, it changes the way that compiled ES6 classes which extend (what babel assumes to be) native constructors such that the subclasses no longer call into the parent classes' constructors. This is allows the transformation to avoid a specific type of error when extending native constructors: most must be constructed with new (or super / Reflect.construct, which aren't available in ES2015) and will throw when called using call or apply. However, the polyfill needs the user to call into the polyfilled constructor to retrieve the exact element which is being upgraded and so that the user's constructors are called with that specific element as this.

This PR manually assembles the set of transforms in @babel/preset-es2015 (with our particular configuration) for the purpose of being able to pin @babel/plugin-transform-classes to v7.0.0-beta.35 .

tests

babel/babel#7506 - a bug about this issue
babel/babel#7533 - related PR to fix bug above

…m-classes can be pinned to v7.0.0-beta.35 .

aomarks · 2018-04-24T21:51:36Z

packages/build/src/js-transform.ts

-    [require('@babel/preset-es2015'), {modules: false}];
+const customBabelPresetEs2015 = {
+  plugins: [
+    [require("@babel/plugin-transform-template-literals"), { loose: false, spec: false }],


I think all of these parameters are default false except for the regenerator ones? Could just leave off the ones that aren't changing a default.

You were right, almost all of these were the defaults.

aomarks · 2018-04-24T21:52:14Z

packages/build/src/js-transform.ts

@@ -29,8 +29,30 @@ import isWindows = require('is-windows');
 // minifying but not compiling?

 // Syntax and transform plugins for ES2015.


Add note linking to bug about class plugin and mention its why we aren't using the preset.

aomarks · 2018-04-24T21:55:15Z

packages/build/src/js-transform.ts

@@ -168,7 +190,7 @@ export function jsTransform(js: string, options: JsTransformOptions): string {
  }
  if (options.compileToEs5) {
    doBabelTransform = true;
-    presets.push(babelPresetEs2015NoModules);
+    presets.push(customBabelPresetEs2015);


plugins.push(...? Probably works either way, but seems more clear now that it's not technically a preset.

AFAICT, this would change the order? https://babeljs.io/docs/plugins/#pluginpreset-ordering

https://babeljs.io/docs/plugins/#creating-a-preset kinda makes it sound like this counts as a preset.

I think a preset is just an array of plugins, and yes it does affect ordering (if you look above in the transform functions I mentioned how things are ordered). I guess this is fine, and it definitely won't change the ordering!

aomarks · 2018-04-24T22:21:44Z

Also mind adding a CHANGELOG entry? Should probably mention it on build, CLI, and polyserve since the bug affects all of those end uses, and each will need to be released to pick it up because we do = pinning on build and the other pre- packages.

aomarks · 2018-04-24T22:31:46Z

If there's a straightforward way to check that the bad transform is not happening, mind adding a regression test in js-transform_test?

bicknellr · 2018-04-24T23:14:00Z

I've added a test that checks for whether or not the empty function on this line (the broken constructor) is injected into the output when creating a class that extends from HTMLElement.

bicknellr · 2018-04-24T23:17:59Z

packages/build/package.json

+    "@babel/plugin-external-helpers": "^7.0.0-beta.46",
+    "@babel/plugin-proposal-async-generator-functions": "^7.0.0-beta.46",
+    "@babel/plugin-proposal-object-rest-spread": "^7.0.0-beta.46",
+    "@babel/plugin-syntax-async-generators": "^7.0.0-beta.46",
    "@babel/plugin-syntax-dynamic-import": "^7.0.0-beta.43",


I'm going to bump this one to 46 also, just to keep everything else at the same version.

…ther babel dependencies.

aomarks · 2018-04-24T23:18:46Z

packages/build/src/test/js-transform_test.ts

+    `);
+    const result = jsTransform(input, {compileToEs5: true});
+
+    assert.notInclude(result, "function Wrapper() {}");


What about checking for wrapNativeSuper, since it's the name of the helper function that contains this line? Maybe less brittle.

Yeah, that's a better idea; the whitespace in this string was making me a bit worried: function Wrapper() {} vs. function Wrapper(){} etc.

WDYT about checking for both?

(the wrapper function thing and wrapNativeSuper)

I think just wrapNativeSuper is fine. If you did do both, make sure it's an or, since an and would be more brittle.

Ok, I'll just do wrapNativeSuper then.

…tructor.

aomarks

Nice!

aomarks · 2018-04-24T23:30:33Z

We should probably test with shop or something, just to make sure it actually works at runtime. Like I could imagine a breakage due to the helpers and the transformer being out of sync.

bicknellr · 2018-04-24T23:34:34Z

I've been trying with a polymer init-created PSK, manually modified to force the custom elements polyfill and w/ UA spoofed to IE11. I think it's working as expected but I'm going to ask @keanulee to double check for me since he has better knowledge of the conditions that lead to this problem.

keanulee

Tested branch locally, LGTM

bicknellr · 2018-04-24T23:55:21Z

@keanulee thanks! I was just about to walk over and ask. :)

usergenic

Travis looks good. Only a couple of uncorrelated WCT test timed out on different VMs. Definite transient errors; doesn't warrant another run at CI if there's no more changes for this PR.

bicknellr · 2018-04-25T18:44:23Z

I'm going to revert the last commit. I was hoping it was going to make these pass but since it's not helping here I don't see any reason to include it.

…ansform-classes

usergenic · 2018-04-26T00:45:42Z

if you merge in master you'll get unit and integration tests split out. maybe easier to see whats going on

…ansform-classes

bicknellr · 2018-04-26T00:46:10Z

Ok, I'll try that.

usergenic · 2018-04-26T02:06:50Z

Russell, I updated travis.yml on master to reduce sauce session contention and got 4 green travis runs on the first try in a long time. Merge master again for that goodness.

usergenic · 2018-04-26T02:08:54Z

@bicknellr I went ahead and merged master into this branch to get that effect. Hopefully will just see green CI in 20 minutes here.

bicknellr · 2018-04-26T02:46:38Z

@usergenic I just saw that PR - awesome! Anxiously awaiting the Travis results: so far 3 for 3.

bicknellr · 2018-04-26T03:04:00Z

Thanks so much for figuring that out!

javatheist · 2018-05-21T16:47:00Z

Pls unpin.

…cates (#216) * Properly key converted documents by original url, correct destination * remove unneccesary template string wrapper * respond to justin review feedback * handle ConvertedDocumentFilePath conversion better * create shouldConvertDocument() method * update upsteam test format

Manually assemble @babel/preset-es2015 so that @babel/plugin-transfor…

1f75c36

…m-classes can be pinned to v7.0.0-beta.35 .

bicknellr requested review from usergenic and aomarks April 24, 2018 21:48

googlebot added the cla: yes label Apr 24, 2018

aomarks requested changes Apr 24, 2018

View reviewed changes

Remove explicit plugin arguments which match defaults.

96c9863

Update CHANGELOG.md; add comment explaining custom preset-es2015.

e096439

Test jsTransform for babel's broken native constructor wrappers.

793269d

bicknellr commented Apr 24, 2018

View reviewed changes

Bump @babel/plugin-syntax-dynamic-import to v7.0.0-beta.46 to match o…

6d70035

…ther babel dependencies.

aomarks reviewed Apr 24, 2018

View reviewed changes

Check for _wrapNativeSuper instead of trying to find the empty cons…

4924961

…tructor.

aomarks approved these changes Apr 24, 2018

View reviewed changes

Update integration test fixtures.

53a593f

keanulee approved these changes Apr 24, 2018

View reviewed changes

bicknellr mentioned this pull request Apr 25, 2018

CE polyfill is incompatible with babelHelpers class adapter webcomponents/custom-elements#157

Closed

bicknellr force-pushed the pin-babel-plugin-transform-classes branch 2 times, most recently from 722388e to e04cf2f Compare April 25, 2018 00:46

usergenic approved these changes Apr 25, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into pin-babel-plugin-tr…

2db915c

…ansform-classes

bicknellr force-pushed the pin-babel-plugin-transform-classes branch from e04cf2f to 2db915c Compare April 25, 2018 18:47

bicknellr added 2 commits April 25, 2018 15:50

Merge remote-tracking branch 'origin/master' into pin-babel-plugin-tr…

f02daa1

…ansform-classes

Merge remote-tracking branch 'origin/master' into pin-babel-plugin-tr…

a790c8e

…ansform-classes

Merge remote-tracking branch 'origin/master' into pin-babel-plugin-tr…

170512b

…ansform-classes

Merge branch 'master' into pin-babel-plugin-transform-classes

0678bf5

bicknellr merged commit 3c2ac01 into master Apr 26, 2018

bicknellr deleted the pin-babel-plugin-transform-classes branch April 26, 2018 03:03

web-padawan mentioned this pull request Apr 26, 2018

[polyserve] Regression: babel compilation breaks CE polyfill in IE11 and Safari 9 #182

Closed

bicknellr mentioned this pull request May 17, 2018

Babel v7 (rc) prevents the custom elements polyfill from finding element definitions. #398

Closed

web-padawan mentioned this pull request Jun 10, 2018

Webpack : Lit Element new usage lit/lit-element#72

Closed

TimvdLippe mentioned this pull request Aug 21, 2018

this.constructor incorrect inside wrapped native constructors. babel/babel#8496

Closed

FredKSchott mentioned this pull request Jan 3, 2019

[Megathread] Refactor ProjectConverter to Fix URL Handling #2721

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pin @babel/plugin-transform-classes to v7.0.0-beta.35 #216

Pin @babel/plugin-transform-classes to v7.0.0-beta.35 #216

bicknellr commented Apr 24, 2018 •

edited

Loading

aomarks Apr 24, 2018

bicknellr Apr 24, 2018

aomarks Apr 24, 2018

bicknellr Apr 24, 2018

aomarks Apr 24, 2018

bicknellr Apr 24, 2018

bicknellr Apr 24, 2018

aomarks Apr 24, 2018

aomarks commented Apr 24, 2018

aomarks commented Apr 24, 2018

bicknellr commented Apr 24, 2018 •

edited

Loading

bicknellr Apr 24, 2018

aomarks Apr 24, 2018

bicknellr Apr 24, 2018

bicknellr Apr 24, 2018

bicknellr Apr 24, 2018

aomarks Apr 24, 2018

bicknellr Apr 24, 2018

aomarks left a comment

aomarks commented Apr 24, 2018

bicknellr commented Apr 24, 2018

keanulee left a comment

bicknellr commented Apr 24, 2018

usergenic left a comment

bicknellr commented Apr 25, 2018

usergenic commented Apr 26, 2018

bicknellr commented Apr 26, 2018

usergenic commented Apr 26, 2018

usergenic commented Apr 26, 2018

bicknellr commented Apr 26, 2018

bicknellr commented Apr 26, 2018

javatheist commented May 21, 2018

		@@ -29,8 +29,30 @@ import isWindows = require('is-windows');
		// minifying but not compiling?

		// Syntax and transform plugins for ES2015.

Pin @babel/plugin-transform-classes to v7.0.0-beta.35 #216

Pin @babel/plugin-transform-classes to v7.0.0-beta.35 #216

Conversation

bicknellr commented Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aomarks commented Apr 24, 2018

aomarks commented Apr 24, 2018

bicknellr commented Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aomarks left a comment

Choose a reason for hiding this comment

aomarks commented Apr 24, 2018

bicknellr commented Apr 24, 2018

keanulee left a comment

Choose a reason for hiding this comment

bicknellr commented Apr 24, 2018

usergenic left a comment

Choose a reason for hiding this comment

bicknellr commented Apr 25, 2018

usergenic commented Apr 26, 2018

bicknellr commented Apr 26, 2018

usergenic commented Apr 26, 2018

usergenic commented Apr 26, 2018

bicknellr commented Apr 26, 2018

bicknellr commented Apr 26, 2018

javatheist commented May 21, 2018

bicknellr commented Apr 24, 2018 •

edited

Loading

bicknellr commented Apr 24, 2018 •

edited

Loading