ESM all the things

[lucacasonato]

This PR changes all internal code (scripts/, test/, util/) to ESM. The
built package for NPM now contains an ESM and CJS entrypoint.

ESM is supported in all LTS Node versions now.

[ddbeck]

This is interesting, @lucacasonato. Thanks for opening it. I've got a couple of questions for you.

I do quite like the import/export syntax over CommonJS, though I'll have to admit to some ignorance here on the advantages of ESM generally. Was there anything in particular that motivated this PR?

And on this point in particular:

The built package for NPM now contains an ESM and CJS entrypoint.

This is interesting and reminds me of a problem reported when we started pre-building the package in the first place, #10033. One thing I've noticed in this PR is that the built package contains the data twice: once as data.json and once again as the default export of index.js. Do you know if there's a way we might avoid doubling the size of package?

[ddbeck]

Oh, a side note: I haven't tested it yet, but upgrading ora to 6.0.0 (#12164) would probably be unblocked by merging this PR.

[lucacasonato]

I do quite like the import/export syntax over CommonJS, though I'll have to admit to some ignorance here on the advantages of ESM generally. Was there anything in particular that motivated this PR?

Yeah. The main motivation was making sure the module would be directly consumable by browsers or Deno. (import bcd from "https://unpkg.com/@mdn/browser-compat-data/index.js"). As long as it uses require, that doesn't work.

Also tooling is generally moving in the direction of ESM, so would be good to keep the project up to date in that sense. Many developers just starting out with JS will feel a lot more familiar with ESM compared to CJS.

One thing I've noticed in this PR is that the built package contains the data twice: once as data.json and once again as the default export of index.js. Do you know if there's a way we might avoid doubling the size of package?

Indeed - this is an unfortunate side-effect caused by the lack of support for importing JSON files directly in ESM right now. This can be resolved in a couple of months once import assertions have stabilized across the ecosystem. The type: json assertion is already supported in Chrome, but not yet in Node, Deno or TSC.

Once import assertions have gained stability across the ecosystem, the ESM entrypoint can be changed to the following:

import bcd from "./data.json" asserts { type: "json" };
export default bcd;

[ddbeck]

Thanks for the additional info, @lucacasonato. That's quite helpful.

@Elchi3 do you have an opinion on whether we ought to take on that size penalty? The package used to be quite a bit larger anyway, so maybe it's small price to pay for expanding the environments that BCD works in.

[ddbeck]

@foolip and @vinyldarkscratch: I think you've been most in the weeds for the internal tooling. Any quick impressions on switching to ES modules? I think this might impact a couple of other PRs y'all have open.

[queengooborg]

I can't really say that I have much experiences in the benefits and downsides to using ES modules vs. CommonJS require(). I've personally stuck with CommonJS for most projects since I'm more familiar with it, and from what I've read online it seems to have kept pretty consistent across all NodeJS versions.

@lucacasonato is there a particular problem you're trying to solve that's led you to want to switch to ESM for everything?

In regards to adding ESM bindings to the releases, I am definitely down for that. Can we split that out into its own PR for easier and quicker review?

[lucacasonato]

@lucacasonato is there a particular problem you're trying to solve that's led you to want to switch to ESM for everything?

Yeah, mentioned them 3 comments up (#12161 (comment)). Additionally @ddbeck brought up a good point about ora moving to ESM only, so not being able to update it anymore: #12164

In regards to adding ESM bindings to the releases, I am definitely down for that. Can we split that out into its own PR for easier and quicker review?

Will do! See #12169

[Elchi3]

@Elchi3 do you have an opinion on whether we ought to take on that size penalty? The package used to be quite a bit larger anyway, so maybe it's small price to pay for expanding the environments that BCD works in.

I think it's fine but we could check in with the folks that were particularly interested in a smaller package size. I think it was @bershanskiy who investigated this in an earlier PR. Anton, let us know if you have thoughts :)

[bershanskiy]

I think it's fine but we could check in with the folks that were particularly interested in a smaller package size. I think it was @bershanskiy who investigated this in an earlier PR. Anton, let us know if you have thoughts :)

Thanks for notifying me of this change. I do like ESM better over CJS, but doubling of produced output size is unfortunate.

Please note that the source and the output can evolve independently. That is, the source and tests (scripts/, test/, util/) can fully migrate to ESM without creating any changes in the produced output. This freedom to alter source code, test infrastructure, supported Node versions, and everyting else was one of the expressed goals of bundling.

One thing I've noticed in this PR is that the built package contains the data twice: once as data.json and once again as the default export of index.js. Do you know if there's a way we might avoid doubling the size of package?

Indeed - this is an unfortunate side-effect caused by the lack of support for importing JSON files directly in ESM right now. This can be resolved in a couple of months once import assertions have stabilized across the ecosystem. The type: json assertion is already supported in Chrome, but not yet in Node, Deno or TSC.

Import assertions are an important aspect of ESM. Since import assertions should roll out across the ecosystem in a couple months, may be we should continue old style of releases for now and then migrate to ESM once they mature?

[queengooborg]

@lucacasonato is there a particular problem you're trying to solve that's led you to want to switch to ESM for everything?

Yeah, mentioned them 3 comments up (#12161 (comment)). Additionally @ddbeck brought up a good point about ora moving to ESM only, so not being able to update it anymore: #12164

Ah! Apologies, I had initially reviewed this PR on my iPhone, so I hadn't seen any of the earlier comments. 😅 I'm all for providing consumer-facing ESM bindings, so that we may maximize compatibility with ease! For our internal code, if our dependencies are switching to becoming ESM-only, then I'll give a +1 for migrating to ESM ourselves, especially since it has no impact on our consumers.

@foolip and @vinyldarkscratch: I think you've been most in the weeds for the internal tooling. Any quick impressions on switching to ES modules? I think this might impact a couple of other PRs y'all have open.

This might cause some merge conflicts with PRs we have open, but since it's all in regards to the imports up top, resolving the conflicts seems straightforward, so I'm not worried about this!

[foolip]

@foolip and @vinyldarkscratch: I think you've been most in the weeds for the internal tooling. Any quick impressions on switching to ES modules? I think this might impact a couple of other PRs y'all have open.

I'm pretty sure this will require https://github.com/foolip/mdn-bcd-collector to use ESM as well. I gave that a shot once but gave up because of some dependency or error I couldn't be bothered to figure out. But I am going to put off converting mdn-bcd-collector indefinitely until something is broken, so if it's BCD that breaks it and I have to do the work, that's fine with me.

[Elchi3]

If it requires consumers to upgrade to ESM, should we aim to get this in a 5.0.0 release?

[queengooborg]

I think this should land in a release with a major server bump, since it is a potentially backwards compatibility breaking change; I explained a reason why in the cherry-picked #12169 PR. With these changes, however, almost every consumer will be able to continue using CommonJS, since Luca has added exported ESM bindings alongside the current CJS ones for maximum compatibility.

The case of mdn-bcd-collector is a more special case, which is why we would need to switch to ESM with these changes, but it does suggest a potential compatibility break. Unlike most consumers, our project is designed to write changes to BCD, so it interacts with a local clone of the repository to create modifications to the existing JSON files. This means that it is interacting with the contributor-facing index.js rather than the bundled version.

To resolve this, we could have a separate index.js for ESM and another for CJS, much like how pre-built bundles would be set up in #12169?

[ddbeck]

Coming back to this briefly: having read the discussion, I want to highlight @bershanskiy:

Please note that the source and the output can evolve independently.

Import assertions are an important aspect of ESM. Since import assertions should roll out across the ecosystem in a couple months, may be we should continue old style of releases for now and then migrate to ESM once they mature?

These are both good points. To unblock this PR, here's what I'm thinking:

• Go ahead with ESM-ing everything on the development side of the package boundary. I don't see any objections to that component, at least. It's a big chunk of the work and would unblock that dependabot PR. And, if I understand correctly, that might allow for BCD-from-source to work under Deno.
• Look at ESM-ing the stuff on the other side of the package boundary separately. Perhaps there's something we can do with pre-releases and/or npm release tags to offer the oversized ESM version of the package (e.g., offering up v4.1.0.esm-dev alongside the standard releases) separately from the regular package, until import assertions become more widely available?

@lucacasonato what do you think?

[queengooborg]

Could I request we wait on ESM-ing BCD's internals until I've converted mdn-bcd-collector? Since the collector project interacts with BCD on a write-basis, we need to import the development version instead, so we need to follow along with the ESM conversion, and it'll take a little longer than normal due to our testing setup.

(Note: we are a special case! Normal BCD consumers will not have to worry about this, since they are using pre-built bundles!)

[ddbeck]

@queengooborg

Could I request we wait on ESM-ing BCD's internals until I've converted mdn-bcd-collector?

Apart from giving you some time to do it, do you need anything else on the BCD side of things to make this easier?

[caugner]

LGTM, just minor nits.

[lucacasonato]

This looks good to me now.

[queengooborg]

LGTM overall, thank you! Just a small nit regarding the readme:

[queengooborg]

Can we revert this change, since BCD isn't ESM-ready yet?

[caugner]

Just to clarify, this change would have to wait until the first ESM-compatible version of BCD is released, right?

PS: I guess that will be a major version bump?

[queengooborg]

That's correct, see #15775 for the ESM-ready public builds!

[queengooborg]

Oh, before we merge this PR by the way, I'd like to land #15944 first. In my testing, I found that Ajv's "strict mode" warnings get repeated for every run of the schema validation in ESM, so errors get buried underneath the warnings. Why this only occurs on ESM, I'm not sure. That PR has landed so this is good to go again, aside from the above nit!

[github-actions]

This pull request has merge conflicts that must be resolved before we can merge this.

[foolip]

@queengooborg there are lots of conflicts again. Would this be ready to merge after another rebase? I haven't reviewed it at all, it's so big :)

[queengooborg]

Once a rebase and my above review comment is addressed, this PR will be ready, yes!

[queengooborg]

I went ahead and performed a rebase (amongst a few small changes to satisfy ESLint) to get this ready for merging. I was also holding off on merging this myself since I had (originally accidentally) contributed to this PR, but I realize we had gotten co-owner preapproval for this anyways.

Thank you @lucacasonato for all your hard work on this, let's get it merged! 👍