Implement `setClassMethods` assumption #12407

nicolo-ribaudo · 2020-11-27T15:26:16Z

Q	A
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
License	MIT

Main PR: #12219
RFC: babel/rfcs#5

Affected plugins:

transform-classes

babel-bot · 2020-11-27T15:29:35Z

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

nicolo-ribaudo · 2020-11-27T15:35:14Z

packages/babel-plugin-transform-classes/src/index.js

@@ -21,6 +21,8 @@ export default declare((api, options) => {

  const { loose } = options;

+  const setClassMethods = options.loose || api.assumption("setClassMethods");


Which option should have the precedence? loose is set directly in the plugin, but assumption is the new modern option.

I think loose should be prioritized. We can use api.assumption("setClassMethods"); as default value for loose. So loose: false can not be overridden by api.assumption("setClassMethods");.

Do we want some kind of warning somewhere if both are specified?

codesandbox-ci · 2020-11-27T15:38:09Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2a785bb:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

hzoo

at least this part of the implementation is straightforward, 👍

jridgewell · 2020-12-05T04:44:14Z

packages/babel-plugin-transform-classes/src/index.js

@@ -21,6 +21,8 @@ export default declare((api, options) => {

  const { loose } = options;

+  const setClassMethods = options.loose ?? api.assumption("setClassMethods");


Actually, can we flip this? Have api.assumption return undefined unless is explicitly specified, then default to loose:

api.assumption("setClassMethods") ?? options.loose

I think the assumption should be the highest priority, and we fall back to the old loose flag if the assumption isn't set.

The rationale for giving precedence to loose is that, even if it's "older", it's closer to the plugin:

{ "assumptions": { "noDocumentAll": false }, "plugins": [ "@babel/proposal-nullish-coalescing-operator", ["@babel/proposal-optional-chaining", { "loose": true }] ] }

I think that the most intuitive thing would be to respect loose for @babel/proposal-optional-chaining (and thus output == null), because it's explicitly passed to that specific plugin.

We can deprecate loose option in Babel 8 and rename loose in optional chaining to noDocumentAll. Otherwise it is unclear for users that loose is actually meant to override the noDocumentAll assumption.

So the question is if the top level thing (assumptions) should override the one closer to a plugin.. well either way we should notify the user somehow right? We don't want to suggest that both should be on right?

~~IMO~~ @JLHwung In that case, it should go:~~

Suggested change

const setClassMethods = options.loose ?? api.assumption("setClassMethods");

const setClassMethods =

options.setClassMethods ??

api.assumption("setClassMethods") ??

options.loose;

Half of the goal of the assumptions RFC is to centralise this kind of option rather than keeping it per-plugin, since (especially with class plugins) they have big cross-plugin implications.

I agree that loose is "closer" to the plugin, but the assumptions list is highly specific to transformer output. So I see it as being the higher precedence item.

Plus, I can see toggling certain assumptions on/off and using loose at the same time. This would allow devs to override some of the parts of loose, eg enabling classCallCheck while otherwise outputting loose code.

~~For backward compatibility to Babel < 7.13, we should check if api.assumption is available.~~

It has been handled in https://github.com/babel/babel/pull/12219/files#diff-876e2533ff9db5901b423e389d5aee5275594e79da63f0cdc14c8ce62f90b468R31

JLHwung

If transformClass is not public, we should eventually replace isLoose by assumptions.

- transform-classes

- `mutableTemplateObject` and `ignoreToPrimitiveHint` (#12408) - `setClassMethods` (#12407) - `setComputedProperties` (#12490) - `ignoreFunctionLength` (#12491) - `noDocumentAll` (#12481) - `iterableIsArray` and `arrayLikeIsIterable` (#12489) - `pureGetters` (#12504) - `skipForOfIteratorClosing` (#12496) - `objectRestNoSymbols`, `setSpreadProperties` and `pureGetters` (#12505) - `noNewArrows` (#12613, #12793) - `setPublicClassFields` and `privateFieldsAsProperties` (#12497) - `constantReexports` and `enumerableModuleMeta` (#12618) - `constantSuper`, `superIsCallableConstructor` and `noClassCalls` (#12726) Co-authored-by: Justin Ridgewell <justin@ridgewell.name> Co-authored-by: Huáng Jùnliàng <JLHwung@users.noreply.github.com>

nicolo-ribaudo added Spec: Classes area: assumptions labels Nov 27, 2020

nicolo-ribaudo mentioned this pull request Nov 27, 2020

Add @babel/core support for the new assumptions option #12219

Merged

2 tasks

nicolo-ribaudo force-pushed the assumption/classes branch from af2cd67 to 013d938 Compare November 27, 2020 15:32

nicolo-ribaudo force-pushed the nicolo-ribaudo/assumptions branch from 0ba8512 to 9b673e3 Compare November 27, 2020 15:32

nicolo-ribaudo commented Nov 27, 2020

View reviewed changes

nicolo-ribaudo force-pushed the assumption/classes branch from 013d938 to a4f47b8 Compare November 27, 2020 15:48

nicolo-ribaudo force-pushed the nicolo-ribaudo/assumptions branch 2 times, most recently from da76ea2 to c12cc57 Compare November 27, 2020 15:52

nicolo-ribaudo force-pushed the assumption/classes branch from a4f47b8 to bfa7eef Compare November 27, 2020 16:03

hzoo approved these changes Dec 4, 2020

View reviewed changes

jridgewell approved these changes Dec 5, 2020

View reviewed changes

jridgewell reviewed Dec 5, 2020

View reviewed changes

jridgewell mentioned this pull request Dec 5, 2020

Implement mutableTemplateObject and ignoreToPrimitiveHint #12408

Merged

nicolo-ribaudo mentioned this pull request Dec 8, 2020

Replace "loose" options with a top-level "assumptions" object babel/rfcs#5

Merged

nicolo-ribaudo added the ⚠️ Do not merge label Dec 8, 2020

nicolo-ribaudo force-pushed the nicolo-ribaudo/assumptions branch from c12cc57 to 8affe9b Compare December 10, 2020 20:27

nicolo-ribaudo force-pushed the assumption/classes branch from 0c915a5 to f32c34e Compare December 10, 2020 23:03

nicolo-ribaudo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Dec 10, 2020

JLHwung approved these changes Dec 10, 2020

View reviewed changes

existentialism approved these changes Dec 11, 2020

View reviewed changes

nicolo-ribaudo removed the ⚠️ Do not merge label Dec 11, 2020

Implement setClassMethods

2a785bb

- transform-classes

nicolo-ribaudo force-pushed the assumption/classes branch from f32c34e to 2a785bb Compare December 11, 2020 19:15

nicolo-ribaudo changed the base branch from nicolo-ribaudo/assumptions to feat-7.13.0/babel-core-features December 11, 2020 19:15

nicolo-ribaudo merged commit ee9deaa into babel:feat-7.13.0/babel-core-features Dec 11, 2020

nicolo-ribaudo deleted the assumption/classes branch December 11, 2020 19:29

nicolo-ribaudo mentioned this pull request Dec 11, 2020

New Babel 7.13.0 features for @babel/core #12470

Merged

24 tasks

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jan 12, 2021

Implement setClassMethods assumption (babel#12407)

695b60a

nicolo-ribaudo added a commit that referenced this pull request Jan 24, 2021

Implement setClassMethods assumption (#12407)

4fdccdd

nicolo-ribaudo added a commit that referenced this pull request Jan 24, 2021

Implement setClassMethods assumption (#12407)

f69e918

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Feb 4, 2021

Implement setClassMethods assumption (babel#12407)

ce8cf51

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Feb 10, 2021

Implement setClassMethods assumption (babel#12407)

4c9a96e

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Feb 11, 2021

Implement setClassMethods assumption (babel#12407)

af453ab

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 13, 2021

github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `setClassMethods` assumption #12407

Implement `setClassMethods` assumption #12407

nicolo-ribaudo commented Nov 27, 2020 •

edited by gitpod-io bot

babel-bot commented Nov 27, 2020 •

edited

nicolo-ribaudo Nov 27, 2020

JLHwung Nov 27, 2020

hzoo Dec 4, 2020

codesandbox-ci bot commented Nov 27, 2020 •

edited

hzoo left a comment

jridgewell Dec 5, 2020

nicolo-ribaudo Dec 5, 2020 •

edited

JLHwung Dec 5, 2020

hzoo Dec 5, 2020

ExE-Boss Dec 7, 2020 •

edited

nicolo-ribaudo Dec 7, 2020

jridgewell Dec 7, 2020

JLHwung Dec 10, 2020 •

edited

JLHwung left a comment

		@@ -21,6 +21,8 @@ export default declare((api, options) => {

		const { loose } = options;

		const setClassMethods = options.loose \|\| api.assumption("setClassMethods");

		@@ -21,6 +21,8 @@ export default declare((api, options) => {

		const { loose } = options;

		const setClassMethods = options.loose ?? api.assumption("setClassMethods");

-  const setClassMethods = options.loose ?? api.assumption("setClassMethods");
+  const setClassMethods =
+    options.setClassMethods ??
+    api.assumption("setClassMethods") ??
+    options.loose;

Implement setClassMethods assumption #12407

Implement setClassMethods assumption #12407

Conversation

nicolo-ribaudo commented Nov 27, 2020 • edited by gitpod-io bot

babel-bot commented Nov 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesandbox-ci bot commented Nov 27, 2020 • edited

hzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo Dec 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss Dec 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLHwung Dec 10, 2020 • edited

Choose a reason for hiding this comment

JLHwung left a comment

Choose a reason for hiding this comment

Implement `setClassMethods` assumption #12407

Implement `setClassMethods` assumption #12407

nicolo-ribaudo commented Nov 27, 2020 •

edited by gitpod-io bot

babel-bot commented Nov 27, 2020 •

edited

codesandbox-ci bot commented Nov 27, 2020 •

edited

nicolo-ribaudo Dec 5, 2020 •

edited

ExE-Boss Dec 7, 2020 •

edited

JLHwung Dec 10, 2020 •

edited