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

Support newer ECMAScript version #382

Open
Indy-rbo opened this issue Aug 9, 2024 · 12 comments
Open

Support newer ECMAScript version #382

Indy-rbo opened this issue Aug 9, 2024 · 12 comments
Assignees
Labels
area/ecmascript Relates to ecmascript module effort/high priority/medium triage/blocked This issue is blocked by another, specified in the description triage/needed Needs to be discussed by project maintainers

Comments

@Indy-rbo
Copy link
Contributor

Indy-rbo commented Aug 9, 2024

Description

We often run into issues with using unsupported JavaScript functions. While there are Shims for some useful functions, it would be nice to be able to utilize newer JavaScript functionality and not have to find work-arounds or write a new Shim.

For example: the Build Tools do support Array.includes (which was added in ECMAScript 2016) via a Shim, but it does not support the Number functions added in ES6 (ECMAScript 2015) (like .isInteger()).

Maybe it'd be possible to integrate core-js into the Build Tools to polyfill new functions during transpilation? Next.js also uses it to polyfill. This would also remove the burden of creating and maintaining correct Shims.

Alternatives

@VenelinBakalov VenelinBakalov added triage/needed Needs to be discussed by project maintainers area/ecmascript Relates to ecmascript module priority/medium effort/high labels Aug 9, 2024
@Michaelpalacce
Copy link
Collaborator

I see this as a few step process. First we need to ensure that the current implementation is 100% unit tested, in a manner that would allow us to experiment with other polyfills. Second would be the potential adoption of core-js

@Michaelpalacce Michaelpalacce added triage/needs-clarification The issue needs further clarification from the contributor and removed triage/needed Needs to be discussed by project maintainers labels Aug 15, 2024
@Michaelpalacce
Copy link
Collaborator

We aren't sure if core-js will work under rhino. We need to upload it and play around with it to see if everything works.

@Michaelpalacce Michaelpalacce self-assigned this Aug 15, 2024
@Indy-rbo
Copy link
Contributor Author

Indy-rbo commented Aug 19, 2024

Looking through the issues on the Rhino repository, it often mentions core-js being used to test certain functionality.
They also have a Release Step called Update core-js-compat
https://github.com/mozilla/rhino/blob/cbb0e2f17019e75eae9f3bc04745dcba4f5a0bac/RELEASE-STEPS.md?plain=1#L127

It seems there is a compat table made by core-js which shows the version that supports the given feature:

https://github.com/zloirock/core-js/blob/master/packages/core-js-compat/src/data.mjs

Hope that helps!

@Michaelpalacce
Copy link
Collaborator

So a small update. I was testing things around and well 2 things happened:

  1. We can't compile the minified JS, so we gotta just push it there directly.
  2. image this is what happens when I run the action I created with all the polyfll. I tried to get just parts of it, but I couldn't seem to make that work either. I went through the source code but I can't extract JUST what I need either

@Indy-rbo
Copy link
Contributor Author

I'm not sure how you currently import the polyfill or what version of Rhino it is using, but I would think this error is to be expected when using modules that are not needed/supported.

Looking at the core-js compat data.mjs:
Core-js has 480 modules, of which 146 have a Rhino tag. The lowest version shown is 1.7.13

image

The core-js docs show it importing a specific module like import 'core-js/actual/promise'; so it should be possible, but I'm not sure how to get the actual code that the import generates.

The core-js-compat package has a target which could be set to the Rhino version to retrieve all the supported module names. Not sure if that is of help for future use: https://github.com/zloirock/core-js/tree/master/packages/core-js-compat#targets-option

@Michaelpalacce
Copy link
Collaborator

Hey @Indy-rbo so I came to that same conclusion very fast.

The way a polyfill should work (to my understanding) is I take the code, that should work well on IE 10 and 11 or whatever, old versions of JS, paste the minified code and my code should be decorated (aka new prototypes added to existing objects, or things have been overwritten and new objects added). This however is not the case.

There are a few things preventing that. First: that some objects are Intentionally Object.seal-ed... I came up randomly upon a few as I was experimenting and Some of them could not be modified. Second: Not all of the code is importable actually, as you saw even the rhino team only import some of the functionalities.

What I have to do in order to get this working is to take the RAW implementation and just use that. I tried this, but the code is full of imports and whatnot, logic is spread around places. It's not like I can take the "Array" implementation in its entirety and just use that. If this is the case, then things get simplified. Either way, at the end of the day, we need to manually figure out what works and what does not.

Now this is just the problem of getting the code to the environment. Second is the issue of actually using it. Currently, we detect usages of specific Words like Set and Map and such, and we insert the shims... that'll have to be rethought a little bit, I fear. Problem is, we may end up having to import everything and not just what we need, or the implementation may be too complex.

Third is the issue of actually supporting the polyfills. That itself is a really complex problem that I am not sure we want to undertake.

Fourth is the issue that polyfills may change native objects to do what Javascript does internally, not what is deemed right by the Rhino engine. I am not fully sure if they do that, but if they do, we may not want to do it. We have a transpiler to transpile TS/JS to vRO's JS code with all of its quirks, not change that entirely.

What we can do though is enhance our current Shims, and we may consider adding a few more. Either way, this needs to be discussed further internally.

@Michaelpalacce Michaelpalacce added triage/needed Needs to be discussed by project maintainers and removed triage/needs-clarification The issue needs further clarification from the contributor labels Aug 23, 2024
@Indy-rbo
Copy link
Contributor Author

Indy-rbo commented Aug 26, 2024

The mentions I see of Rhino in this repository mention that org.mozilla rhino has version 1.7R4.
The latest, best supported version is 1.7.15 according to https://mozilla.github.io/rhino/compat/engines.html. The ECMAScript compatibility between these versions jumps from 6% to 46% (depending on the ES version you are looking at in the table)!

If the version mentioned in this repo is correct, then according to the compat table of core-js this should indeed not work. Since the lowest version mentioned in the table is 1.7.13

It might indeed be a challenge of setting up the importing of specific parts. Not sure how to tackle that.
I would think for the fourth issue is related to using the Shims mentioned in the compat table.

Is it possible to upgrade the Rhino version? This would natively support many of the current Shims

@pboychev-bcom
Copy link

On the topic above I spent some time to do a bit of research, here's my thoughts on that below.

I consider our end goal as the following set of criteria:

  • Provide backwards compatibility - old projects need to be built producing the same behaviour of the code.
  • Newer JavaScript features should be made available in the development.
  • The changes should attempt to make it easier to support more input code versions (JS versions / features) and simplify the support for transpiling that to a moving target - depending on the version of the Rhino engine in the Aria Auto Orchestrator (currently Rhino 1.7R4); the idea being if and when the engine is changed / updated in the product we can easily add support for that new JS version.
  • When in time we need to introduce even newer features of JS it should be easy to do so.
  • The changes should produce reliable and stable code, the implementation should be well tested.

I spent a bit of time going through the code as it currently is and thinking about possible solutions for our case and an option worth considering for me is to try and make use of Babel for the job.
Why:

  • It is built for transforming / transpiling JavaScript versions - it's an exact match for what we need.
  • It is using core-js - mentioned in the comments above.
  • It is well tested and widely used, mature.
  • It provides support for multiple source and target versions of JavaScript.
  • Polyfills are added on demand, should not result in imports of redundant definitions.
  • Should provide support for newer JS versions / features further in time, community is strong.

I spent a bit of time playing around with transpiling code snippets with async/await, Promises, anonymous functions as constants, class definitions, spread operators, destructuring objects and arrays etc. and testing if the resulting code works in JavaScript 1.7 - the version supported in Rhino 1.7R4. All seemed fine.
When tested the same code, however I hit issues due to the delta between the JavaScript 1.7 standard and what is implemented in Rhino.

What I would propose to further investigate is the following:

  • Can we replace our current transpilation to a large degree with Babel based transpilation - what we need to do on top / customize in the Babel code transpilation in order to make it work in the Rhino runtime. This might prove to be quite complex and time consuming, as well as with a large dose of uncertainty. If achievable however, we should be able to reduce any custom shims and other techniques of introducing JS features support to a minimum. Hopefully we should be able to get the source version and the target version and let Babel do its thing. After that, however we should still take care of handing imports etc.
  • Plug the Babel transpilation in our current implementation - instead of writing our own shims we could plug the Babel transpilation at the right time in our current process, e.g. after the code was transpiled by the tsc (but without adding our current shims). This will fix issues with the compatibility between what Babel produces when transpiling ES6+ to JS 1.7 and what Rhino supports - an example of such that I found issue with is the class definitions, Babel changes those to functions who's prototype is being decorated to add the rest of the properties and methods and Rhino does not allow such thing.

If one of the two options above are successful, I'd even suggest to think about transpiling the "*.js" actions, not just the TypeScript definitions. This will enable newer JS features in all our code, not just TS. And since Babel is transpiling JS to JS we should get backwards compatibility for older, already existing actions - if the syntax is compatible with JS 1.7 Babel will have nothing to do.

@Michaelpalacce, @Indy-rbo, feel free to share your view on this approach and we can decide if we'd like to give it a try.

@Indy-rbo
Copy link
Contributor Author

Indy-rbo commented Sep 9, 2024

I love the idea of integrating Babel and think it's a great way forward for the future of the Build Tools.

Great write-up, you already mentioned many great points about the benefits of this approach. I have nothing more to add, but am very curious about what the maintainers think of this idea.

@VenelinBakalov
Copy link
Collaborator

We already had an internal discussion, the outcome was the summary mentioned above by Plamen.
Unfortunately at the current situation such a huge change would not be managable by the team but we decided to keep the topic open and to add the summary of the best identified approach here in case:

  1. In the future we have capacity and are able to mitigate this change
  2. There is an external contributor who is interested and able to implement this approach

@pboychev-bcom
Copy link

@Indy-rbo I will take this discussion further internally to clarify what how etc., already spoke with @VenelinBakalov, I believe we're on the same page. :)
Estimating the effort is in progress.

Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 20, 2024
@VenelinBakalov VenelinBakalov added triage/blocked This issue is blocked by another, specified in the description and removed Stale labels Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ecmascript Relates to ecmascript module effort/high priority/medium triage/blocked This issue is blocked by another, specified in the description triage/needed Needs to be discussed by project maintainers
Projects
None yet
Development

No branches or pull requests

4 participants