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 Babel Configuration to Support toSorted() Method #7571

Open
hussain-t opened this issue Sep 11, 2023 · 10 comments
Open

Update Babel Configuration to Support toSorted() Method #7571

hussain-t opened this issue Sep 11, 2023 · 10 comments
Labels
Module: Analytics Google Analytics module related issues P2 Low priority Type: Enhancement Improvement of an existing feature

Comments

@hussain-t
Copy link
Collaborator

hussain-t commented Sep 11, 2023

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

Implementation Brief

  • Add [email protected] 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

@hussain-t hussain-t self-assigned this Sep 11, 2023
@hussain-t hussain-t added Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues labels Sep 11, 2023
@hussain-t hussain-t removed their assignment Sep 11, 2023
@hussain-t hussain-t added the P2 Low priority label Sep 11, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Sep 12, 2023
@tofumatt
Copy link
Collaborator

ACs 👍🏻

@nfmohit
Copy link
Collaborator

nfmohit commented Oct 23, 2023

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
Copy link
Collaborator

techanvil commented Oct 24, 2023

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:

transform: {
'^.+\\.[jt]sx?$': '<rootDir>/tests/e2e/babel-transform.js',
},

module.exports = babelJest.createTransformer( {
presets: [ '@wordpress/babel-preset-default' ],
} );

use: [
{
loader: 'babel-loader',
options: {
sourceMap: mode !== 'production',
babelrc: false,
configFile: false,
cacheDirectory: true,
presets: [ '@wordpress/default', '@babel/preset-react' ],
},
},

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
Copy link

mxbclang commented Jan 8, 2024

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

@benbowler benbowler assigned benbowler and unassigned nfmohit Jan 31, 2024
@benbowler
Copy link
Collaborator

benbowler commented Feb 2, 2024

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

  1. 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.
  2. core-js/es-shims - would need work to wrap as a babel preset.
  3. 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: #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
Copy link
Collaborator

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/[email protected]. 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 [email protected] 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/[email protected] upgrades to core-js@^3.31.0. Although, with @wordpress/[email protected] depending on @wordpress/[email protected] (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 #6356, #6357 and #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 [email protected]. 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
Copy link
Collaborator

Thanks @techanvil, I had some more time to look at this today and you were correct that [email protected] 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.

@benbowler benbowler assigned techanvil and unassigned benbowler Feb 9, 2024
@techanvil
Copy link
Collaborator

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?

@techanvil techanvil removed their assignment Feb 13, 2024
@benbowler
Copy link
Collaborator

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.

@benbowler benbowler removed their assignment Feb 14, 2024
@techanvil techanvil self-assigned this Feb 14, 2024
@techanvil
Copy link
Collaborator

techanvil commented Feb 14, 2024

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.

@techanvil techanvil removed their assignment Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P2 Low priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants