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

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

Merged
merged 30 commits into from
Feb 21, 2021

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 7, 2020

View Rendered Text


These are the issues mentioned in the RFC. I'm putting them here so that GitHub links them.
babel/babel#11622 babel/babel#11248 babel/babel#6978 babel/babel#11689 babel/babel#11634

@developit
Copy link
Member

developit commented Jun 9, 2020

Love the visibility this brings into what "loose" was actually doing.

Some questions
  • would it be worth providing a boolean "assume all" option?
    • { assumption: "all" } or { assumptions: true }
  • if the list of assumption keys is large, will it be necessary to add groups?
    • TypeScript sortof has this with downlevelIteration
  • what happens when an option is false, but the surrounding code provides enough context to make a given assumption?
    • Example: for-of over an array literal
Naming bikeshedding thoughts

I wondered if it might be helpful to have a consistent "[feature] with [caveat]" nomenclature:

usePlusForTemplatestemplatesAsStrings
noFrozenTemplateObjectmutableTemplateStrings
setObjectSpreadPropertiesspreadAsProperties
definePrivateAsOwnPropertiesprivateFieldsAsProperties
inheritsLooseinheritAsObjectCreate
setModuleMetacjsDefaultExport
immutableReexportsreexportAsProperties
noNewArrowAttemptarrowsAsFunctions

@existentialism
Copy link
Member

existentialism commented Jun 9, 2020

Fantastic work on compiling all the current options!

re: Open Question:

  1. Yeah. Will landing #11689 let us pass a full list?
  2. My gut/initial reaction is that we should validate the list of assumptions
  3. Yeah, I think we should default to no assumptions, so user's can own (and with improved documentation) all the assumptions/tradeoffs that come with enabling each option.
  4. I think it would be awesome to tie top level targets with assumptions, but we'll have to make sure we can notify users when their target's assumptions conflict with the ones they set? (but not be annoying?)

re: option name 🚲 🏠 ing, I actually love @developit's proposed scheme!

@hzoo
Copy link
Member

hzoo commented Jun 12, 2020

"assume all" option?
we should default to no assumptions

Would be equivalent to loose: true in babel 5? Given our current state is spec by default, I think most people, if they want to use loose at all, would choose to opt-out of specific things vs. opt-in to specific?

The list will most likely grow, and it is already large, I think we may need to remind people that again you can share this in an object, or a config somewhere. Probably may even write code comments to explain the options. for templatesAsStrings, // a${b} -> 'a' + b

Agreed on naming, ideally feature name is similar to plugin name?

Here's a related issue/PR regarding plugin options twbs/bootstrap#31011

@nicolo-ribaudo
Copy link
Member Author

Thanks y'all for taking the time to read this!

@developit

would it be worth providing a boolean "assume all" option?

I would prefer not to do it, for two main reasons:

  1. When you enable it you still have no idea of what it's doing
  2. It will keep us in the current position where we can't add more "looseness" to loose, because it is a breaking change.

if the list of assumption keys is large, will it be necessary to add groups?

I initially thought about something like this:

"assumptions": {
  "templates": { "usePlus": true, "noFrozenObject": true }
}

However, I now think that a flat list of options is easier to manage and to document. Also, it would create a new problem: should we deep-merge assumptions, or only Object.assign it their top-level object?

We could still have conceptual groups by starting the name of related with the same word, like templateAsStrings and templateObjectMutable.

what happens when an option is false, but the surrounding code provides enough context to make a given assumption?

I'd say that if we can be sure that it can be optimized, we should do it even if the assumption is set to false (e.g. for (const x of [1, 2, 3]); is iterating over an array, we are not assuming it).
I don't really want false and "unset" to be two different things, otherwise it wouldn't be a boolean anymore.

I wondered if it might be helpful to have a consistent "[feature] with [caveat]" nomenclature:

I like this idea! I personally don't like your proposed option names because I found them not descriptive (sorry 😅): templates are always string, spread always introduces properties, arrows are always converted to functions, etc. However, I will see if I can rename the options following this pattern.


@existentialism I'll reply to your points tomorrow!

@nicolo-ribaudo
Copy link
Member Author

@existentialism

  1. Yeah. Will landing #11689 let us pass a full list?

That PR will let us pass a full list to plugins. It's impossible to pass the final assumptions to presets if we allow presets to enable assumptions.

  1. My gut/initial reaction is that we should validate the list of assumptions

I initially wasn't 100% on board with it because it means that if we add support for a new assumptions in a plugin, then the user would need to update @babel/core in order to use it (otherwise Babel would throw an "unknown assumption" error).
However, I think that the benefits of validating outweigh this downside 👍

  1. I think it would be awesome to tie top level targets with assumptions, but we'll have to make sure we can notify users when their target's assumptions conflict with the ones they set?

Could you give an example of a possible conflict? 🙏

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Oct 19, 2020

I tried using @developit's proposed naming scheme, but while some of those names are better than what I proposed, they are not always descriptive. While "doing some research", I actually understood what loose mode does and I think I have names that better describe not Babel's loose output, but the behavior of that output.

usePlusForTemplatestemplatesAsStrings

Templates always generate strings, and templatesAsStrings doesn't explain neither what the effect on the generated output is (it transforms "a${b}c" to "a" + b + "c") nor what the runtime effect is:

var o = { [Symbol.toPrimitive]: h => h };

`hint: ${o}` === "hint: string";

"hint: ".concat(o) === "hint: string"; // correct
"hint: " + o; === "hint: default"; // wrong (loose mode)

Maybe ignoreToPrimitiveHint? When this is on, it can be also used for other things if they rely on @@toPrimitive's hint (for example, the old decorators proposal IIRC).

noFrozenTemplateObjectmutableTemplateStrings

I propose mutableTemplateObject, since it's the object that is mutable (strings are always immutable). mutable is better than noFrozen.

(obj => {
  "use strict";
  obj.x = 2; // in loose mode, this doesn't throw
})``;

setObjectSpreadPropertiesspreadAsProperties

Here the key is that in loose mode we use [[Set]] rather than [[Define]] for copying spread properties. Maybe setSpreadProperties? It's shorter than what I originally proposed, but it still has the same meaning (since "object" is implied by "properties").

// This should be 2, but it's 3 in loose mode
({ set x(v) { this.y = 3 }, get x() { return this.y }, ...{ x: 2 } }).x;

definePrivateAsOwnPropertiesprivateFieldsAsProperties

privateFieldsAsPublic?

The difference in loose mode is that

Object.getOwnPropertyNames(new class A { #x }) === ["__private_0_x"]

inheritsLooseinheritAsObjectCreate

👍 inheritAsObjectCreate, or maybe extendClassAsObjectCreate?

setModuleMetacjsDefaultExport

I prefer to keep setModuleMeta here, since it's again a [[Set]] vs [[Define]] case. This is the difference:

// input (main.js)
import * as mod from "./mod.js";

console.log(Object.keys(mod));

// input (mod.js)
export {};

// output (mod.js - spec)
"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});

// output (mod.js - loose)
"use strict";

exports.__esModule = true;

In loose mode it logs ["__esModule"], in spec mode [].

immutableReexportsreexportAsProperties

No strong preference here, both seem equally good/bad to me

noNewArrowAttemptarrowsAsFunctions

Technically arrows are functions 😛 Maybe newableArrowFunctions?


```typescript
type Assumptions = {
[assumption: string]: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the default values of any assumption is false? The newableArrowFunctions can be a footgun since the assumption is more spec-compliant. Maybe we can have noNewableArrowFunctions and make it defaults to true?

Copy link
Member Author

Choose a reason for hiding this comment

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

My plan for this is to default to true for compat in Babel 7, and default to false in Babel 8 (since currently it's the only thing where spec compliancy is opt-in)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also consider deprecate spec of preset-env/transform-arrow-functions and encourage to use assumptions, so we can eventually remove spec in Babel 8.

rfcs/0000-top-level-assumptions.md Outdated Show resolved Hide resolved
## Configuration merging

Multiple `"assumptions"` objects should be merged using `Object.assign`, and not overwritten like other options.
This makes it easy, for example, to enable one additional assumption for a specific folder. Also, it still keeps the ability of disabling an assumption simply by setting it to `false`.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to the RFC text


```typescript
type Assumptions = {
[assumption: string]: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another question: would assumptions allow options? If so merging assumptions will be challenging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I described them as simple booleans exactly to avoid thinking about merging them 😅

I feel like we can just use longer names instead of nesting objects.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I am too permissive, but think about how ESLint rules config starts from { [string]: boolean } and end with { [string]: [number, object] }. 😂 We will see.


- This RFC introduces a very big number of new top-level options (19), and makes it relatively cheap to add new ones. Having a big number of options can add more "tooling fatigue" on the shoulders of our users, and it can make it hard for us to properly document them. However, these new options replace 22 existing plugin options, many of which have the same name (`loose`) but different behaviors.

- ([@JLHwung](https://github.com/JLHwung)) Some `assumptions` are only effective in one plugin, but users will have to go through their presets/plugins to see if they have opt-in to this plugins. Let's say there is an assumption in a stage-1 proposal and the users does not know it is no-op for their config, it may inadvertently turn into a long list of `assumptions` and the no-op assumptions can kick in once users upgrades `preset-env` which may includes it once its stage advances.
Copy link
Collaborator

Choose a reason for hiding this comment

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

On possible assumptions in early stage plugins, can we limit assumptions to be a something that related to stage 3 features or standard spec only? So we don't add assumptions about stage-1 plugins until they are mature enough. The assumptions can still be a valid plugin-specific optionl.

Copy link
Member Author

Choose a reason for hiding this comment

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

I 99%* agree! Maybe we should have the same policy we have for shippedProposals?

* only 99% because, for example, the #foo in obj transform is obviously influenced by the assumptions we make when transforming private fields (privateFieldsAsPublic).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In your case privateFieldsAsPublic falls to shipped feature assumptions. A stage-1 proposal implementation can choose to respect such assumptions, but it can not, i.e. register a global privateInNeverNested assumption, until it is mature enough -- So other plugins might have to take care of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

100% agree now 😄

I'll add a "Policy" section.

- The option name
- It's description
- An example (which could be interactive, using CodeSandbox)
- The affected plugins
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe worthy to offer a green engines list for each assumptions. Like if you are not transpiling down to these browsers, this assumption is a no-op for you. i.e. People should not worry about what is setSpreadProperties if they are not traspiling object spread anyway.

Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Oct 19, 2020

Choose a reason for hiding this comment

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

We could even automatically generate it by computing the union of all the affected plugins' targets maybe

# Open questions

- Should we try to pass to the presets at least a _partial_ `assumptions` object? Currently `@babel/preset-env` relies on `loose` to enable/disable the `typeof-symbol` plugin.
- Should we validate the list of `assumptions` in `@babel/core`, and disallow unknown ones? This would make it impossible for third-party plugins to introduce their own assumptions, but it also means that it's easier for us to introduce new assumptions without risking ecosystem incompatibilities.
Copy link
Member Author

Choose a reason for hiding this comment

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

#5 (comment) (@existentialism)

  1. My gut/initial reaction is that we should validate the list of assumptions

#5 (comment) (@nicolo-ribaudo)

I initially wasn't 100% on board with it because it means that if we add support for a new assumptions in a plugin, then the user would need to update @babel/core in order to use it (otherwise Babel would throw an "unknown assumption" error).
However, I think that the benefits of validating outweigh this downside 👍


I was going to write in the RFC text that @babel/core will throw on unknown assumptions, but I realized that if we add a new assumptions in @babel/[email protected], presets cannot use it because it would be a breaking change when used with @babel/[email protected].

Note that this problem will likely never affect our own presets (since we won't enable assumptions in our own presets), but it could affect framework-specific ones.

We have a few options:

  1. Decide that it's ok, presets can check Babel's version and conditionally enable an assumption.
  2. Only throw for unknown assumption in config files, not in presets.
  3. Throw in config files, warn in presets.
  4. Always warn.
  5. Don't validate the assumptions list.

In any case, we shouldn't pass unknown assumptions down to plugins.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Note that @babel/preset-env also validate unknown options. If a preset depends on @babel/preset-env, every time when a new option is introduced, they have to bump dependencies, too. I think it applies to assumptions of @babel/core, too.

This was referenced Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants