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

Update compat data (specifically for rhino 1.7.14) #14208

Merged
merged 2 commits into from Jan 27, 2022

Conversation

phulin
Copy link
Contributor

@phulin phulin commented Jan 26, 2022

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

This is a first attempt at updating compat data, in this case specifically to support rhino 1.7.14. Per discussion over at mozilla/rhino#1164 with @nicolo-ribaudo, I updated the fixtures as well with make build-compat-data && make bootstrap && OVERWRITE=true yarn jest. Unfortunately, this seems to have triggered a number of unrelated changes to the fixture stdout files and caused one of the fixture tests to fail:

 FAIL  packages/babel-preset-env/test/fixtures.js (12.192 s)
babel-preset-env/shipped proposals > new class features chrome 94
        SyntaxError: /Users/phulin/Documents/Projects/kol/babel/packages/babel-preset-env/test/fixtures/shipped-proposals/new-class-features-chrome-94/input.js: Static class blocks are not enabled. Please add `@babel/plugin-proposal-class-static-block` to your configuration.
        > 1 | class A {
            | ^
          2 |   #foo;
          3 |
          4 |   static {

Curious for any advice you all have as to how to resolve these. I'm sure I'm doing something wrong here.

@phulin phulin marked this pull request as draft January 26, 2022 20:13
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jan 26, 2022

Uh, it's because of #14181 / compat-table/compat-table#1784. We'll handle that issue soon-ish, but for now you can edit

"proposal-class-properties": {
features: [
"static class fields",
"instance class fields, public instance class fields",
"instance class fields, private instance class fields basic support",
"instance class fields, computed instance class fields",
],
to explicitly list the other static fields test (and remove the generic "static class fields"):

"static class fields, public static class fields",
"static class fields, private static class fields",
"static class fields, computed static class fields"

@nicolo-ribaudo nicolo-ribaudo added area: compat-table PR: Dependency ⬆️ PR: Internal 🏠 A type of pull request used for our changelog categories labels Jan 26, 2022
@phulin
Copy link
Contributor Author

phulin commented Jan 26, 2022

Huh, that doesn't seem to have addressed either the test failure or the extraneous stdout.txt changes. Any other ideas?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jan 26, 2022

It didn't work because the separator is / (with one space before and one after), not , 😬
"static class fields / public static class fields", etc.

@phulin
Copy link
Contributor Author

phulin commented Jan 26, 2022

Perfect, thank you. That appears to have worked correctly. I didn't commit the changes to plugin-features.js as I assume those are temporarily required only.

@phulin phulin marked this pull request as ready for review January 26, 2022 21:41
@nicolo-ribaudo
Copy link
Member

Oh please commit them, so that the committed script always reflects the committed data.

@babel-bot
Copy link
Collaborator

babel-bot commented Jan 26, 2022

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

@phulin
Copy link
Contributor Author

phulin commented Jan 26, 2022

Done. Thanks for the help.

"static class fields",
"static class fields / public static class fields",
"static class fields / private static class fields",
"static class fields / computed static class fields",
"instance class fields, public instance class fields",
Copy link
Member

Choose a reason for hiding this comment

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

I will open another PR changing , to / here. I'd prefer to keep it separate because it might cause changes to our compat data and fixtures not related to rhino.

@nicolo-ribaudo nicolo-ribaudo changed the title Update compat data (specifically for rhino 1.7.14). Update compat data (specifically for rhino 1.7.14) Jan 27, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit 0bef6c1 into babel:main Jan 27, 2022
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Apr 29, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: compat-table outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Dependency ⬆️ PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants