Merge class features plugins #8130

nicolo-ribaudo · 2018-06-08T09:01:26Z

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR
Any Dependency Changes?
License	MIT

cc @loganfsmyth @jridgewell I tried to move plugin-proposal-class-properties to the new "big" plugin. All the original tests are still passing 🎉

I split the class properties transformation logic in different functions, so that it can be reused. For example, to transform decorators we will need to reuse buildPrivateNamesMap, buildPrivateNamesNodes, transformPrivateNamesUsage and injectInitialization.

Questions: (next meeting?)

Should we also merge @babel/plugin-transform-classes?
- Pro: It makes it a lot easier to reuse the class IIFE. It can be accomplished anyway, but in an uglier way (#6993).
- Con: What will we do if/when we will implement the classes-1.1 proposal? It doesn't fit with this @babel/plugin-proposal-enhanced-classes plugin because every feature is not compatible, so we would probably need to create another plugin which depends on this one with only class transpilation enabled.
- NOTE: This can be done later.
What will we do when a proposal implemented by this plugin becomes part of preset-env?
- Should we just remove -proposal- from this plugin's name now? e.g. @babel/plugin-enhanced-classes or @babel/plugin-transform-enhanced-classes.
Should it be possible to enable loose for the different features separately?
- This would probably make sense only if we merge the class transform plugin.
- NOTE: This can be added later if we don't merge the classes transform. Otherwise, it will be a breaking chance because both transform-classes and proposal-class-properties can be loose separately (this can be probably worked-around though).
How much specific the options should be?
- One option per proposal? (instanceFields, staticFieldsAndMethods, instancePrivateMethods?
- Make a "grid"? (instance, static - public, private - fields, methods, accessors)
- Very specific? (publicInstanceFields, privateStaticMethods, ...)
- NOTE: Options can be added/divided/merged later.
Name bikeshedding: @babel/plugin-proposal-enhanced-classes? @babel/plugin-proposal-class-features?

NOTE: This PR only "merges" the class properties plugin, but it creates the whole infrastructure.

babel-bot · 2018-06-08T09:09:57Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9414/

Andarist · 2018-06-08T23:18:01Z

packages/babel-plugin-proposal-enhanced-classes/README.md

@@ -0,0 +1,19 @@
+# @babel/plugin-proposal-enhanced-classes


it's not obvious for me how this is supposed to be used (by looking into README), could you add some simple example here?

I can't 😛 (#8108)

I will open a new PR containing the docs when this PR is more ready

Andarist · 2018-06-08T23:41:39Z

packages/babel-plugin-proposal-class-properties/src/index.js

-        path.insertBefore(computedNodes);
-        path.insertAfter(staticNodes);
-      },
+    inherits(babel) {


can this be used in combination with enhanced classes? or is it using enhanced-classes or class-properties (where latter is ofc just a thin config wrapper over former)? could we validate misusage somehow?

class-properties is meant to be a "switch" which turns on cass fields in the enhanced-classes plugin. If enhanced-classes hasn't already been added by the user, class-properties will do it.

But can they be used side by side? Would enhanced-classes turned on by class-properties be "merged" with "standalone" enhanced-classes? In other words - is enhanced-classes' instance shareable among such invocations?

Suppose a user uses both class-properties and deorators. Each plugin will

Register a new enhanced-classes plugin

Turn on the coresponding feature in an option set in the shared map in the File class.

The first instance enhanced-classes will run, transforming both decorators and class fields because they are both enabled in the shared map.
The second instance of enhanced-classes will run, but it will be a no-op because the classes have already been transformed.

We can't currently disable the second plugin (babel-core doesn't offer this ability), but I think that we can live with the first run doing "everything" and the other beeing no-op.

Andarist · 2018-06-08T23:45:16Z

Whats the story now behind enhanced-classes and transform-classes? Have u considered merging them too? Atm it's hard to do some transformations which requires coordination between transform-classes & class-properties - would love to solve this.

Andarist · 2018-06-09T11:28:49Z

Also - what will happen once i.e. class properties get standardised? Will it stay as part of enhanced-classes?

Jessidhia · 2018-06-09T11:31:38Z

I suspect they have to stay transformed because of decorators. Though it could be possible to add a mode to make the plugin not transform properties if the class isn't decorated.

Andarist · 2018-06-09T12:02:50Z

I suspect they have to stay transformed because of decorators.

I would suggest to merge this with transform-classes to avoid such ambiguity altogether.

nicolo-ribaudo · 2018-06-10T19:43:21Z

Whats the story now behind enhanced-classes and transform-classes? Have u considered merging them too? Atm it's hard to do some transformations which requires coordination between transform-classes & class-properties - would love to solve this.

I didn't do it yet because classes have been part of JS for a long time, while in this plugin I wanted to merge the proposals about class fields, private methods, static class features and decorators (which are considered like a "family" of proposal, since they depend on each other). We could do it if it simplifies classes transformation.

Also - what will happen once i.e. class properties get standardised? Will it stay as part of enhanced-classes?

This plugin would stay as-is, maybe we will remove the classFields option and it will only be possible to enable it using the @babel/plugin-transform-class-fields switcher.

Andarist · 2018-06-10T19:54:31Z

Either way is fine to me (although I believe it would make certain transformations easier if we would just merge all class-related transforms into a single plugin). I feel like there should be a clear path defined what will happen in the future regarding those. I think we might be pretty certain that all of those (class fields, private methods, decorators) will get to the language - so it's only a matter of time. And knowing that let us define now what should be done in the future in regards of those transforms. We kinda have an opportunity here to avoid users being confused in the future by moved parts etc.

Andarist · 2018-06-12T07:47:50Z

Im going to discuss further here - because it's imho annoying to reply under "hidden" section of comments.

From what I understand - loose is shared between all of the options here, right? Would be cooler to let users specify it for each feature separately.

nicolo-ribaudo · 2018-06-13T08:46:20Z

From what I understand - loose is shared between all of the options here, right? Would be cooler to let users specify it for each feature separately.

We could do something like this:

["@babel/plugin-proposal-enhanced-classes", {
  "loose": true, // Applies to all the features, but it can be overwritten
  "privateMethods": true, // loose, as specified before
  "instanceFields": { "loose": false } // spec compliant
}]

I'm not 100% sure that we should, though. Decorators: why would someone want to have, for example, private instance properties in loose mode (without the weakmap), but private methods spec compliant? Also, the decorators don't have their own loose mode, but they need to be loose if, for example, instance fields or static fields are loose.

Andarist · 2018-06-13T15:16:02Z

If it complicates things too much, then I guess this is not a blocker of any kind. I was thinking about the idea of merging this with regular class transform and in such situation it's easier to imagine a situation where 1 would like to use spec classes but i.e. loose class fields.

What do u think anyway about the idea of merging them?

Andarist · 2018-07-13T08:51:48Z

Should we also merge @babel/plugin-transform-classes?

👍 IMHO it should be done, it simplifies transformations around classes.

Con: What will we do if/when we will implement the classes-1.1 proposal? It doesn't fit with this @babel/plugin-proposal-enhanced-classes plugin because every feature is not compatible, so we would probably need to create another plugin which depends on this one with only class transpilation enabled.

Couldnt it be just another option of the plugin that couldnt be used in combination with other options?

Should it be possible to enable loose for the different features separately?

Would be a good feature in places where it makes sense.

How much specific the options should be?

It seems sane to me to just go with 1 option per proposal.

nicolo-ribaudo · 2018-09-05T14:15:36Z

Rebased to include #8463 #8205, #8614 and #8620

nicolo-ribaudo · 2018-09-05T14:17:16Z

.../babel-plugin-proposal-class-properties/test/fixtures/private-loose/native-classes/output.js

 Object.defineProperty(Foo, _foo, {
  writable: true,
  value: "foo"
 });
-
-var _bar = babelHelpers.classPrivateFieldLooseKey("bar");


This is the only change in the output, because I merged the logic for static and instance properties, but it shouldn't be observable.

xtuc · 2018-09-10T16:23:24Z

packages/babel-plugin-proposal-enhanced-classes/package.json

+{
+  "name": "@babel/plugin-proposal-enhanced-classes",
+  "version": "7.0.0",
+  "author": "The Babel Team (https://babeljs.io/team)",


I think this should be Babel contributors, because we have contributors that are not in the team.

Well, there is the "contributors" field for that (https://docs.npmjs.com/files/package.json#people-fields-author-contributors). I think that "author" shouldn't be whoever wrote some parts of the code, but who is responsible for it.

Andarist · 2018-09-12T07:42:25Z

packages/babel-plugin-class-features/package.json

+  "author": "The Babel Team (https://babeljs.io/team)",
+  "license": "MIT",
+  "description": "Compile class public and private fields, private methods and decorators to ES6",
+  "repository": "https://github.com/babel/babel/tree/master/packages/babel-plugin-enhanced-classes",


enhanced-classes -> classes-features everywhere

Andarist · 2018-09-12T07:43:32Z

packages/babel-plugin-class-features/src/decorators.js

@@ -0,0 +1,3 @@
+export function hasDecorators(path) {
+  return path.node.decorators && path.node.decorators.length;


you could cast it to boolean here

Andarist · 2018-10-18T08:27:53Z

what's the status of this PR? is this something that rest of the team wants too? or is it still under consideration?

nicolo-ribaudo · 2018-10-18T08:37:36Z

Yeah, we want this PR but it still needs to be reviewed.

Andarist · 2018-10-18T09:30:52Z

Is there anything I could help with?

nicolo-ribaudo · 2018-10-24T22:04:11Z

Should it be possible to enable loose for the different features separately?

I think that I will move the loose option to fields: { loose: true }, so that it specific for each feature. Otherwise we couldn't have, for example, loose class properties with spec-compliant decorators.

@Andarist

Is there anything I could help with?

You could review this PR. (or, if you already did it, leave a ✔️)

pabloalmunia · 2018-10-25T07:52:34Z

This PR can probably make it easier to fix #8750

The workaround ~~don't~~ work in my cases.

Any progress about the support of HTMLElements decoration in this PR?

hzoo

Overall awesome job!

Also wondering if we should determine if we want to consider making a generic API to get some of this stuff working for other kinds of plugins as a result of this work (having to write all this logic around manipulateOptions, pre, loose etc)

hzoo · 2018-10-26T18:54:04Z

packages/babel-plugin-class-features/src/features.js

+export function verifyUsedFeatures(path, file) {
+  if (hasFeature(file, FEATURES.decorators)) {
+    throw new Error(
+      "@babel/plugin-class-features doesn't suport decorators yet.",


Suggested change

"@babel/plugin-class-features doesn't suport decorators yet.",

"@babel/plugin-class-features doesn't support decorators yet.",

and below

loganfsmyth · 2018-11-07T04:15:31Z

packages/babel-plugin-class-features/src/index.js

+      }
+    },
+
+    pre() {


I'm a bit concerned around how some of this will work if there are multiple versions of things. If we eventually add decorators with their own inherits, we'll have two separate copies of this plugin running. Would we want to allow both to run? We could potentially have one detect that another instance has already loaded or something, but then we still have the question of which one we end up choosing.

For now, only the first plug-in runs (actually both runs, but it is a noop because everything is already transformed).
Probably I should require("package.json"). version and only run the most recent version? Or is it overkill?

Btw, I will merge the decorators transform before the next release, to check that everything works.

The big question for me is if we want plugins like proposal-class-properties and proposal-decorators at the same time as babel-plugin-class-features (which I also notice we may want to be babel-plugin-proposal-class-features)?

For instance, we could also deprecate those two plugins, and instead instruct people to use

plugins: [ ["@babel/proposal-class-features", { decorators: true, publicFields: true, privateFields: true, }] ]

which would more nicely bundle up everything more consistently with our current plugin design.

It's more work for users though, which is why I'm on the fence, but it would mean things are less confusingly side-effectful.

My initial idea was to create a single plugin (I didn't even thought at the current implementation), but different people advocated for keeping the interface as separate plugins.

The big question for me is if we want plugins like proposal-class-properties and proposal-decorators at the same time as babel-plugin-class-features

Unless people rely on exact versions of the class-features plugin (without ^) and, at the same time, npm/yarn doesn't dedupe this package, I don't think that using both class-features and class-properties at the same time would cause confusing effects.
Anyway, we could disallow using them together if you think that it would be better.

which I also notice we may want to be babel-plugin-proposal-class-features

There will be a time when this plugin will transform standard features and proposals (e.g. when class fields become standard). This was one of the reason that the multi-plugins interface was preferred.

I'm going to split up the actual transform into another PR, so we can keep discussing the interface without blocking #8654 and #8248

different people advocated for keeping the interface as separate plugins.

Do you have a link to those discussions? I'm concerned that the approach here will make it really hard to maintain and predict how changes might affect users.

Unless people rely on exact versions of the class-features plugin (without ^) and, at the same time, npm/yarn doesn't dedupe this package, I don't think that using both class-features and class-properties at the same time would cause confusing effects.

Sorry, I should clarify, I don't mean whether they should work together, I mean whether we should maintain both. I'd be tempted to have babel-plugin-proposal-class-features be an entire replacement for proposal-decorators and proposal-class-properties, with those being removed from the monorepo.

There will be a time when this plugin will transform standard features and proposals (e.g. when class fields become standard). This was one of the reason that the multi-plugins interface was preferred.

There hard part there is that in many cases it's not possible to have one without the other. If we keep things bundled in one plugin, at least it is clear to users that this plugin of class props + decorators + private would likely take recedence, so if say class fields stabilized first, preset-env could default-include

['transform-class-features', { publicFields: true }]

Which could throw if it encounters classes that have public fields that are decorated, or private fields.

Then if the user adds

['proposal-class-features', { decorators: true }]

this would essentially override the previous config because it would run first, so it could loudly throw if it encounters public/private fields because they haven't been enabled on that instance of the plugin. Potentially we'd only have to technically error here if a class contained both a decorator and a public field.

Do you have a link to those discussions?

Unfortunately there isn't very much in the meeting notes - the older reference I could find is https://github.com/babel/notes/blob/f58a330d02a2ba7eb29f85dfb88f3c95174d077b/2018/2018-09/sept-10.md#new-class-proposals-decorators-private-fields-etc, which is three months after this PR was opened.

Sorry, I should clarify, I don't mean whether they should work together, I mean whether we should maintain both.

class-properties is only a switch for the class-features plugin, it's not something by itself (https://github.com/nicolo-ribaudo/babel/blob/e70d0f03543058ae804bb7814dd7936ae9e0ace7/packages/babel-plugin-proposal-class-properties/src/index.js).

Then if the user adds

['proposal-class-features', { decorators: true }]

this would essentially override the previous config because it would run first, so it could loudly throw if it encounters public/private fields because they haven't been enabled on that instance of the plugin.

I don't think that this is what a user expects: decorators and class fields are different features (even if they need to be implemented together). Why would enabling decorators disable class fields?
Having the separate "switchers" plugin avoids this confusing behavior: if I enable decorators, it doesn't affect class fields being enabled/disabled.

Btw, the fact that you used both transform and proposal shows that this package's name shouldn't contain either (or should contain both), unless you meant that they should be two separated plugins.

If I had to choose, I would disable using the class-features plugin directly, since I agree that this configuration would be confusing:

plugins: [ ["@babel/plugin-class-features", { "classFields": true }], ["@babel/plugin-class-features", { "decorators": true }], ]

would be surprising if none of them are hidden inside a preset.

xtuc · 2018-11-19T17:06:44Z

packages/babel-plugin-class-features/README.md

+
+```sh
+yarn add @babel/plugin-class-features --dev
+```


This is auto generated btw, see the script in scripts.

xtuc

lgtm, please re-generate the README before

loganfsmyth · 2018-11-19T17:41:22Z

packages/babel-plugin-class-features/src/index.js

+
+    visitor: {
+      Class(path, state) {
+        if (this.file.get(versionKey) !== version) return;


I think we'll want this check on the PrivateName visitor below too.

existentialism · 2018-11-20T21:06:07Z

Great work @nicolo-ribaudo!

nicolo-ribaudo · 2018-11-20T21:08:20Z

Thank you! Merging this PR is so relieving 🙂

Andarist reviewed Jun 8, 2018

View reviewed changes

nicolo-ribaudo added PR: WIP Spec: Class Fields Spec: Decorators PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jun 10, 2018

nicolo-ribaudo force-pushed the merge-class-plugins branch from ad4c05d to 1fe676f Compare June 13, 2018 09:28

nicolo-ribaudo mentioned this pull request Jul 1, 2018

[WIP] Add support for decorators with private properties #8248

Closed

nicolo-ribaudo force-pushed the merge-class-plugins branch from 1fe676f to 9bfb632 Compare July 12, 2018 19:54

nicolo-ribaudo removed the PR: WIP label Jul 12, 2018

nicolo-ribaudo changed the title ~~[wip] Merge class features plugins~~ Merge class features plugins Jul 12, 2018

nicolo-ribaudo force-pushed the merge-class-plugins branch from 9bfb632 to 8399597 Compare July 28, 2018 06:23

nicolo-ribaudo mentioned this pull request Sep 5, 2018

[static private] Use explicit descriptors instead of an object #8620

Merged

nicolo-ribaudo force-pushed the merge-class-plugins branch from 8399597 to 04de64d Compare September 5, 2018 14:13

nicolo-ribaudo commented Sep 5, 2018

View reviewed changes

xtuc reviewed Sep 10, 2018

View reviewed changes

Andarist reviewed Sep 12, 2018

View reviewed changes

andrewmiller1 mentioned this pull request Oct 1, 2018

Map plugins to transformers and vice versa #8790

Closed

Andarist mentioned this pull request Oct 18, 2018

Annotating transformed classes with #__PURE__ comment #6209

Merged

hzoo approved these changes Oct 26, 2018

View reviewed changes

hzoo reviewed Oct 26, 2018

View reviewed changes

nicolo-ribaudo force-pushed the merge-class-plugins branch from c290381 to 9953836 Compare November 2, 2018 14:58

Andarist approved these changes Nov 5, 2018

View reviewed changes

loganfsmyth reviewed Nov 7, 2018

View reviewed changes

This was referenced Nov 14, 2018

[WIP] Private Class Methods Refactor nicolo-ribaudo/babel#1

Closed

Private class methods stage 3 #8654

Merged

xtuc mentioned this pull request Nov 15, 2018

Babel meeting - november 19th babel/notes#73

Closed

6 tasks

xtuc reviewed Nov 19, 2018

View reviewed changes

xtuc approved these changes Nov 19, 2018

View reviewed changes

loganfsmyth reviewed Nov 19, 2018

View reviewed changes

loganfsmyth approved these changes Nov 19, 2018

View reviewed changes

Move class properties transformation logic to enanced-classes

5114047

nicolo-ribaudo force-pushed the merge-class-plugins branch from e70d0f0 to 5114047 Compare November 20, 2018 16:45

nicolo-ribaudo merged commit 5979b06 into babel:master Nov 20, 2018

nicolo-ribaudo mentioned this pull request Nov 20, 2018

Remove direct options to @babel/plugin-class-features #9050

Closed

nicolo-ribaudo added this to Done in Class features Nov 21, 2018

developit mentioned this pull request Dec 11, 2018

Skip setting defaults when calling Object.defineProperty() #9163

Closed

nicolo-ribaudo deleted the merge-class-plugins branch March 4, 2019 23:46

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019

		@@ -0,0 +1,3 @@
		export function hasDecorators(path) {
		return path.node.decorators && path.node.decorators.length;

	"@babel/plugin-class-features doesn't suport decorators yet.",
	"@babel/plugin-class-features doesn't support decorators yet.",

+                    }
+                  },
+                  pre() {

Merge class features plugins #8130

Merge class features plugins #8130

Conversation

nicolo-ribaudo commented Jun 8, 2018 • edited

babel-bot commented Jun 8, 2018 • edited

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

Andarist commented Jun 8, 2018

Andarist commented Jun 9, 2018

Jessidhia commented Jun 9, 2018 • edited

Andarist commented Jun 9, 2018

nicolo-ribaudo commented Jun 10, 2018

Andarist commented Jun 10, 2018

Andarist commented Jun 12, 2018

nicolo-ribaudo commented Jun 13, 2018 • edited

Andarist commented Jun 13, 2018

Andarist commented Jul 13, 2018

nicolo-ribaudo commented Sep 5, 2018

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

Andarist commented Oct 18, 2018

nicolo-ribaudo commented Oct 18, 2018

Andarist commented Oct 18, 2018

nicolo-ribaudo commented Oct 24, 2018

pabloalmunia commented Oct 25, 2018 • edited

hzoo left a 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

nicolo-ribaudo Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

existentialism commented Nov 20, 2018

nicolo-ribaudo commented Nov 20, 2018

nicolo-ribaudo commented Jun 8, 2018 •

edited

babel-bot commented Jun 8, 2018 •

edited

Jessidhia commented Jun 9, 2018 •

edited

nicolo-ribaudo commented Jun 13, 2018 •

edited

pabloalmunia commented Oct 25, 2018 •

edited

nicolo-ribaudo Nov 8, 2018 •

edited

nicolo-ribaudo Nov 8, 2018 •

edited