vmware / build-tools-for-vmware-aria

Build Tools for VMware Aria provides development and release management tools for implementing automation solutions based on the VMware Aria Suite and VMware Cloud Director. The solution enables Virtual Infrastructure Administrators and Automation Developers to use standard DevOps practices for managing and deploying content.
Other
48 stars 24 forks source link

Support newer ECMAScript version #382

Open Indy-rbo opened 3 months ago

Indy-rbo commented 3 months ago

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

Michaelpalacce commented 3 months ago

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 commented 3 months ago

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.

Indy-rbo commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

Indy-rbo commented 3 months ago

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 commented 2 months ago

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:

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:

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:

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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.

github-actions[bot] commented 1 month ago

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.