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

Only allow ECMAScript stage 4 features #22083

Merged
merged 4 commits into from May 18, 2020

Conversation

oandregal
Copy link
Member

While reviewing #22030 (comment), I realized that we're allowing to write code in ECMAScript stage-3 and above, and not only stage-4.

This was enabled by switching on shippedProposals in #19065. At the time, it looks like the thought was that enabling this, will make babel to list feature proposals that were part of stage-4 and already shipped by browsers. However, shippedProposals may add stage-3 as per the docs. For example, at the moment, we could theoretically use the numeric separator feature, which is on stage-3.

This disables the shippedProposals so we only use stage-4.

@oandregal oandregal requested review from aduth and jsnajdr May 4, 2020 15:25
@oandregal oandregal self-assigned this May 4, 2020
@oandregal oandregal added the [Package] Babel preset /packages/babel-preset-default label May 4, 2020
@oandregal
Copy link
Member Author

I don't know if using stage-3 was expected. If it was, this PR can be safely closed and I'm sorry for the noise! However, by looking at this PR, I gather that the common thinking was that we can write code in stage-4 (that is enabled by our babel preset).

@github-actions
Copy link

github-actions bot commented May 4, 2020

Size Change: 0 B

Total Size: 835 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.19 kB 0 B
build/block-library/editor.css 7.19 kB 0 B
build/block-library/index.js 118 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 182 kB 0 B
build/components/style-rtl.css 17 kB 0 B
build/components/style.css 17 kB 0 B
build/compose/index.js 6.67 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.6 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 28.1 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/index.js 7.87 kB 0 B
build/edit-widgets/style-rtl.css 4.69 kB 0 B
build/edit-widgets/style.css 4.69 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.64 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@aduth
Copy link
Member

aduth commented May 4, 2020

This disables the shippedProposals so we only use stage-4.

Can you clarify how this actually works? i.e. Where do we configure Stage 4?

@oandregal
Copy link
Member Author

Can you clarify how this actually works? i.e. Where do we configure Stage 4?

We don't do it directly, but by virtue of using the preset-env.

The preset enables all approved changes (stage-4) as stated in the docs (which are a bit lacking on specifics, they only say "preset that allows you to use the latest JavaScript").

While reviewing the docgen PR I've actually looked at the babel-preset code and this is what I've found:

This is all a bit convoluted but that's what I understand we do by using the preset-env.

@oandregal
Copy link
Member Author

I've traced at least one case where we use syntax enabled by shippedProposals: we'd need to update that code as well.

@aduth
Copy link
Member

aduth commented May 5, 2020

I've traced at least one case where we use syntax enabled by shippedProposals: we'd need to update that code as well.

As far as I know, this code is run directly through Node and isn't polyfilled in any way. I guess it depends whether this support commitment extends beyond just "features we enable in the Babel configuration" and to also include "any code we write".

@aduth
Copy link
Member

aduth commented May 5, 2020

As noted at #22030 (comment) , this should be considered a breaking change, both for @wordpress/babel-preset-default, and likely also for affected upstream packages (notably @wordpress/scripts).

@oandregal
Copy link
Member Author

As per this thread and this conversation we're going ahead with this. We'll wait to merge until the beginning of next week, so we have a full plugin cycle to uncover any unexpected errors and bundle it with the CSS support as well.

@oandregal oandregal added the [Type] Breaking Change For PRs that introduce a change that will break existing functionality label May 6, 2020
@oandregal
Copy link
Member Author

this should be considered a breaking change

do we add the CHANGELOG comment here or is it enough to mark this PR as a breaking change and we take care of that in the package release? cc @gziolo

@gziolo
Copy link
Member

gziolo commented May 6, 2020

do we add the CHANGELOG comment here or is it enough to mark this PR as a breaking change and we take care of that in the package release?

We should include a note in the CHANGELOG to let people know what has changed so they know how to fix it. In fact, the PR that @aduth is working on #22030 includes a note of how such language features (plugins) can be enabled so it might be a good idea to cross-link.

@oandregal oandregal force-pushed the remove/support-for-less-than-stage-4 branch from e04af5f to 510a899 Compare May 15, 2020 10:54
@oandregal
Copy link
Member Author

@gziolo I've added a changelog comment here as well, so this is ready for review.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Given that the change is applied to @wordpress/babel-preset-default – we should call it a braking change there in the first place. In @wordpress/scripts we probably should only mention that @wordpress/babel-preset had breaking changes that might affect those who use the default config. It's tricky :)

@oandregal oandregal force-pushed the remove/support-for-less-than-stage-4 branch from 510a899 to b7a5592 Compare May 18, 2020 10:21
@oandregal
Copy link
Member Author

Added a note in babel-preset-default as well. I still left both notes as "breaking changes" to be extra-communicative and to call attention to this, although it can be reasonably be lowered if that's what we want. I haven't seen any other package using this preset, so I guess we're fine.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Thank you 💯

@oandregal oandregal merged commit c327f19 into master May 18, 2020
@oandregal oandregal deleted the remove/support-for-less-than-stage-4 branch May 18, 2020 11:26
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 18, 2020
@jsnajdr
Copy link
Member

jsnajdr commented May 28, 2020

Turns out I misunderstood what shippedProposals means when I recommended it for the Gutenberg Babel config 🙁

I thought that the flag includes proposals that are past-stage-4, approved and scheduled for the next standard release, but the standard is not published yet.

But the meaning is different: it includes also stage-3 proposals which are implemented and shipping in browsers and are not yet stable. Every proposal needs the experience and feedback from actual implementation and real-world usage before advancing to stage-4 and beyond.

I had a chat with Till Schneidereit from Mozilla, who explained these details to me, and recommended against any blanket rule that includes stage-3 proposals, and recommended using them in production only if there is a real, sizable benefit.

@gziolo
Copy link
Member

gziolo commented May 28, 2020

@jsnajdr, we get the same explanation in #22030 (comment) from Henry Zhu from Babel :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Babel preset /packages/babel-preset-default [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants