Update Babel Configuration to Support `toSorted()` Method

hussain-t commented 1 year ago

Feature Description

While developing #7458, we encountered an issue regarding the Redux state mutation using the state.sort() method in the getAnalyticsConfigByMeasurementIDs selector. We used cloning as a temporary solution to maintain immutability. However, updating the Babel configuration to support the toSorted() method is considered a more efficient and long-term solution for better maintainability and cleaner code.

Update: As discussed below, we are going to hold off on this one until we have upgraded, or are in a position to upgrade @wordpress/babel-preset-default to version 7.22.0 or above. Issues #6026 and #6357 may facilitate this, so have been added as speculative dependencies.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Update the Babel configuration to the version that supports toSorted().
Replace the current use of cloning in the getAnalyticsConfigByMeasurementIDs selector being implemented in #7458 with toSorted() and verify that it works as intended.
The new Babel setup shouldn't break the existing functionalities or introduce regressions.

Implementation Brief

Add core-js@3.28.0 as a dev dependency to for this version to be installed.
Import the core-js toSorted polyfill with import 'core-js/actual/array/to-sorted'; into assets/js/modules/analytics-4/datastore/webdatastreams.js
Replace summaries.sort(... in getAnalyticsConfigByMeasurementIDs with const sortedSummaries = toSorted(... and update the use of summaries below to use sortedSummaries.

Test Coverage

No additional tests are required but confirm that tests can run successfully using node 14. Specifically focussing on the assets/js/modules/analytics-4/datastore/webdatastreams.test.js tests.

QA Brief

Changelog entry

tofumatt commented 1 year ago

ACs 👍🏻

nfmohit commented 10 months ago

I took a deeper look into this and it appears that this doesn't actually have anything to do with the Babel configuration. The Array.prototype.toSorted() MDN browser compatibility section says that it only supports NodeJS 20 and up, not even LTS. Realistically, we use a much older NodeJS version for everything to work in Site Kit (14).

Since it doesn't appear that we will be updating to support this, does this make sense to close this issue?

CC: @tofumatt @techanvil @hussain-t @aaemnnosttv

techanvil commented 10 months ago

Hi @nfmohit, thanks for taking a look at this one. Actually, unless I have missed something myself I do think it should be resolvable via Babel - with the intention to use toSorted() in the client-side code, we'd be running it in Node via Jest, which in turn uses Babel to transpile the code, using the same @wordpress/babel-preset-default package that webpack uses, by the look of it:

https://github.com/google/site-kit-wp/blob/5aafc62bfa9449790264593dba048da530ee54a7/tests/js/jest.config.js#L13-L15

https://github.com/google/site-kit-wp/blob/5aafc62bfa9449790264593dba048da530ee54a7/tests/e2e/babel-transform.js#L6-L8

https://github.com/google/site-kit-wp/blob/5aafc62bfa9449790264593dba048da530ee54a7/webpack.config.js#L181-L191

etc.

I think presets: ["@wordpress/default"] and @wordpress/babel-preset-default resolve to the same package, although it's worth double checking.

The documentation for our current version of this package mentions extending the Babel capabilities via additional plugins... More recent versions also mention a separate polyfill.

So, I guess the thing to do here is to look into what it would take to extend our Babel configuration in a sensible manner. While this issue is focused on toSorted(), it's likely that we could introduce a new set of capabilities alongside it, e.g. note how toSorted() is in the ES2023 list in this polyfill package.

It's arguably a lot of work for the sake of a single small refactor, but the thought of enabling a new set of language features for us to use is quite appealing so I think it would be worthwhile in the long run. Those are my thoughts on the matter, I'm interested to see what others think too.

mxbclang commented 8 months ago

@nfmohit Just a reminder on this one, please. :)

benbowler commented 7 months ago

@techanvil, I've taken a look through this today and the wordpress/babel-preset-default does already provide polyfills for all browsers Wordpress officially supports, therefore the task here really is about how to shim node 14 to be able to run the tests as if they were in a supported browser. In order to do this I've looked through the following possible implementations:

babel-plugin-polyfill-es-shims - can be added into tests/e2e/babel-transform.js however this causes the complete failure of the test suite with Cannot find module 'array.prototype.concat on all/many tests.
core-js/es-shims - would need work to wrap as a babel preset.
es6-shim - doesn't include shim for toSorted.

In my experimentation I couldn't find a clear path for this to work. Point 1 seemed the best approach however it caused all tests to fail as mentioned above. toSorted is only supported in node 20 so it's unlikely we will have this soon. I'm happy to pick this back up if another developer has some pointers on a possible approach to achieve this but for now I'm stuck.

I've pushed a draft PR with the setup I've been using to evaluate options for other developers to evaluate the same: https://github.com/google/site-kit-wp/pull/8216

Small side note, as @techanvil correctly identified@wordpress/default is an alias for wordpress/babel-preset-default so we could unify these in the codebase for clarity.

techanvil commented 7 months ago

Hi @benbowler, thanks for taking a look into this.

With regard to babel-plugin-polyfill-es-shims , the specific error you've described can be resolved by installing the array.prototype.concat NPM module, although it doesn't get things working, additional errors then emerge and I didn't get to the bottom of it in my testing.

Anyhow - looking into it a bit further, I don't think that babel-plugin-polyfill-es-shims is in fact the way to go here. I did link to it in my previous comment, but that was by way of example rather than a recommended direction.

babel-plugin-polyfill-es-shims is currently at version 0.10.2, so not at a stable release, and has relatively low usage at ~2k weekly downloads. Furthermore - as we've seen, it attempts to polyfill Array.prototype.concat() and it looks like it could end up replacing the current polyfill implementation for the supported APIs from ES5 up as listed on the package README.

It would be preferable for our test transpilation config not to diverge too much from dev and production. We might end up inadvertently providing support for language features that are not available in dev/prod, for one thing.

Looking into core-js, which is of course a more mature and well-used library (admittedly with a bit of uncertainty over its long term future, but it's already embedded in our tooling), support for toSorted() was introduced in version 3.28.0, as can be seen in the Changelog.

As alluded to, core-js is already a dependency of @wordpress/babel-preset-default, versioned ^3.6.4 in our currently installed @wordpress/babel-preset-default@4.17.0. It's also a dependency of numerous other installed NPM modules as can be seen with npm ls core-js. We currently have version 3.6.5 installed at the top level of node_modules.

We could install core-js@3.28.0 now and try to get things working to support toSorted() and other languages features at the same ECMAScript release level (taking a look at core-js notes on Babel integration).

However, it might be more appropriate to upgrade to a more recent @wordpress/babel-preset-default - note that @wordpress/babel-preset-default@7.22.0 upgrades to core-js@^3.31.0. Although, with @wordpress/babel-preset-default@7.22.0 depending on @wordpress/element@5.15.0 (which in turn depends on React 18) we'd be dependent on some fairly wide-ranging package upgrades, which we have defined in other issues. See https://github.com/google/site-kit-wp/issues/6356, https://github.com/google/site-kit-wp/issues/6357 and https://github.com/google/site-kit-wp/issues/6026.

Seeing as there's no clear timeframe for getting these upgrades done, I would suggest a bit of investigation into an early upgrade to core-js@3.28.0. If it turns out to be straightforward it would be nice to unlock the new language features. Otherwise I think we can park this until we've addressed the wider upgrades.

benbowler commented 7 months ago

Thanks @techanvil, I had some more time to look at this today and you were correct that core-js@3.28.0 works well and allows assets/js/modules/analytics-4/datastore/webdatastreams.test.js tests to pass with the polyfill imported. As before I created a draft PR here as it warranted experimentation and have linked it to this issue as well as writing the steps in the IB. In this PR all other tests pass and the FE builds without issue. Only the E2E tests on nightly builds are failing at isolates client storage between sessions which will require a little looking at, but may be a wider issue with the E2E against nightly.

techanvil commented 7 months ago

Hey @benbowler, thanks for looking into this further. It's good to see things working, but having to explicitly import the polyfill like that seems a bit cumbersome to put into practice. Really we want something that just gets out of the way and lets us use the language features naturally. Also, for this tooling upgrade to make sense, we should be aiming to provide full JS language support up to and inclusive of ES2023 rather than focusing on toSorted() in isolation.

I had suggested looking into the core-js notes on Babel integration; as I've dug a bit more into this myself I've realised that in fact @babel/preset-env is already geared up to inject core-js polyfills, this can be enabled via config.

So, this would seem like the way forward here, the only problem though is our use of @babel/preset-env is wrapped by @wordpress/babel-preset-default which is in full control of the @babel/preset-env options.

This might seem like a bit of a tangent, the relevance though is that I don't know if it makes sense for us to jump through too many hoops to work around a tooling choice that has effectively been made for us through following a standard WordPress toolchain configuration.

I've also noted that toSorted() is included in the polyfill.js provided by @wordpress/babel-preset-default from version 7.22.0.

So, with all the above in mind, I am inclined to think the right thing to do here is to put this one on hold until we've made the necessary package upgrades to be able to upgrade @wordpress/babel-preset-default to >= 7.22.0, and then weigh up whether we can and want to make use of the provided polyfill.js. This will also include an upgrade to core-js so even if we don't use the provided polyfill.js, we'll have an updated core-js version which will reduce the friction for addition tooling modifications if we want to at that point.

How does that sound to you?

benbowler commented 7 months ago

Hey @techanvil thanks for the review and considered response. I agree with your points and will un-assign myself from this one.

Should we move to Stalled and and perhaps mark it as blocked by tickets such as #6026? And/or create an issue to investigate upgrading @wordpress/babel-preset-default to version 7.22.0.

techanvil commented 7 months ago

Thanks @benbowler. I've moved this to Stalled and added #6026 and #6357 as dependencies, we can keep an eye on them and move this issue forward if and when one of them unblocks it.

I've not created a separate issue to upgrade @wordpress/babel-preset-default, this could happen as a byproduct of the above issues, or we can probably tackle it in this one if not.

google / site-kit-wp