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

Add support for rhino as a compilation target #13448

Merged
merged 13 commits into from Aug 3, 2021

Conversation

gausie
Copy link
Contributor

@gausie gausie commented Jun 10, 2021

Q                       A
Fixed Issues? Fixes #2537 (sort of)
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? 👍
Tests Added + Pass? Not sure where they would go
Documentation PR Link As far as I can see, we don't really need to add any
Any Dependency Changes? No
License MIT

As suggested by @nicolo-ribaudo in mozilla/rhino#661, I've updated compat-table to fully represent the latest version of Rhino and this PR should mean that we can now start targeting it with babel!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 10, 2021

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 8ac5af5:

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

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 10, 2021

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

@zloirock
Copy link
Member

zloirock commented Jun 10, 2021

Could you run core-js-compat test case and add a PR to core-js too?

@gausie
Copy link
Contributor Author

gausie commented Jun 10, 2021

Could you run core-js-compat test case and add a PR to core-js too?

Unfortunately Rhino can't really even run the test runner core-js uses (at least via my naive attempt). Would it be ok to merge this before getting that sorted or are they both needed concurrently?

@zloirock
Copy link
Member

zloirock commented Jun 10, 2021

@gausie you could check the source of the test runner and adapt it to Rhino - it's just some lines. It's required for proper built-ins injection by preset-env, however, it's not a block for this PR.

@gausie
Copy link
Contributor Author

gausie commented Jun 10, 2021

@zloirock Sure, I'll adapt in my local copy for rhino, get the data and discard my edits.

@zloirock
Copy link
Member

zloirock commented Jun 10, 2021

@gausie you could add a Rhino runner to the PR like in compat-table - it could simplify adding new data in the future -)

@gausie
Copy link
Contributor Author

gausie commented Jun 10, 2021

@gausie you could add a Rhino runner to the PR like in compat-table - it could simplify adding new data in the future -)

I think I'll just fix the two issues in Rhino!

@gausie
Copy link
Contributor Author

gausie commented Jun 10, 2021

Here's that PR: zloirock/core-js#942

@gausie
Copy link
Contributor Author

gausie commented Jun 10, 2021

@zloirock whats up with these two in-progress ci jobs? Are they waiting for approval to start somehow?

@zloirock
Copy link
Member

I think that it could be better to ask @nicolo-ribaudo since I'm not an expert in current CI.

@nicolo-ribaudo
Copy link
Member

We only run them manually when changing code that only affects Babel 8 or that might affect spec compliancy of our plugin.

@gausie
Copy link
Contributor Author

gausie commented Jun 10, 2021

Ahha! Well this ain't that 😛

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Could you also add a test in babel-preset-env/tests/fixtures to make sure that the rhino target works?

@JLHwung
Copy link
Contributor

JLHwung commented Jun 12, 2021

We need to bump core-js-compat when zloirock/core-js#942 is published.

@gausie
Copy link
Contributor Author

gausie commented Jun 21, 2021

@JLHwung that has now landed in core-js v3.15.0

@gausie
Copy link
Contributor Author

gausie commented Jun 21, 2021

Ok I've bumped core-js-compat but unsure if I should have bumped core-js too, so I've left it.

@nicolo-ribaudo
Copy link
Member

@gausie I'm preparing a PR to bump all the core-js packages. I prefer to keep it separate from this one because this PR will be merged in the next minor, while bumping core-js is more urgent otherwise our e2e tests will start failing.

@gausie
Copy link
Contributor Author

gausie commented Jun 21, 2021

@nicolo-ribaudo ok, is bumping core-js-compat as I have done ok, or should I undo that last commit?

@nicolo-ribaudo
Copy link
Member

We'll ask you to rebase this PR on main after #13491 is merged (or to merge main in this PR), so reverting the last commit is not necessary but might make it easier to rebase later.

@nicolo-ribaudo
Copy link
Member

I just saw compat-table/compat-table#1731. Maybe for the same reason we should have Rhino patch versions in our data? We might need to update our build script to support them.

@nicolo-ribaudo nicolo-ribaudo added this to the v7.15.0 milestone Jun 21, 2021
@gausie
Copy link
Contributor Author

gausie commented Jun 21, 2021

@nicolo-ribaudo Adding in that patch release is easy since every 1.7 would just need the same change, but I'm not sure how I'd go about updating the build scripts.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jun 21, 2021

Mmh, looking at our build script it looks like it should already work 🤔 Could you try updating the COMPAT_TABLE hash, and let's see if it works.

Btw, we just merged #13491 into main.

@gausie
Copy link
Contributor Author

gausie commented Jun 21, 2021

@nicolo-ribaudo Ok that's all done now

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Sorry, I was not clear enough in my previous comment 😅

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Does rhino support ES Modules? I searched its repo and it seems to me rhino only supports commonjs. If I am wrong, we should also update packages/babel-compat-data/scripts/build-modules-support.js, otherwise this PR lgtm.

@gausie
Copy link
Contributor Author

gausie commented Jun 23, 2021

Does rhino support ES Modules? I searched its repo and it seems to me rhino only supports commonjs. If I am wrong, we should also update packages/babel-compat-data/scripts/build-modules-support.js, otherwise this PR lgtm.

You're correct, for now it only supports commonjs. Woop!

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks! We'll merge this for the next minor release, somewhere in July.

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Jun 23, 2021
@nicolo-ribaudo
Copy link
Member

We'll release 7.15 this week. Thank you!

@nicolo-ribaudo nicolo-ribaudo merged commit 830b99d into babel:main Aug 3, 2021
@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 Nov 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: compat-data pkg: preset-env PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot run standalone compiler from babel-core in Rhino JavaScript engine
5 participants