Migrate from CommonJS to ESM

[PyvesB]

Fixes #6528.

Our badge-maker package was left untouched, it is still a standard CommonJS module, which means there will be no impact on downstream consumers. The rest of the Shields.io codebase imports it as any other NPM package. The GitHub approve-bot action was left out as well, as it has its own separate package.json, it can be migrated in due course.

The automated tools I tried weren't very good, the best I found was Putout convert-commonjs-to-esm. As you'll see, a lot of manual fixing was required afterwards. Some parts of the code needed more changes than others, for example loader.js due to that fact that dynamically importing modules is asynchronous and the structure of imported modules is different. I've tried to break things down in commits describing my thought process, though I doubt reviewing each one individually will be very helpful.

Most test suites seem to be passing locally and the backend loads nicely and is able to service badges.

The frontend is where I'm stuck at the moment. I've got some local changes that should make it compliant once the general setup is working, but I'm struggling to get very far on that bit. The error I'm getting locally is along the following lines:

[frontend] TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for C:...\workspace\shields.cache\tmp-5248-0orlJ0tI72FY

If my understanding is correct the ESM loader is trying to load code generated by Gatsby, but Gatsby generated these files without any extension, which is why they're being rejected. Maybe I'm on the wrong track though, I admittedly really don't have much of an understanding of how our frontend fits together. A lot of folks on the Internet seem to be running into trouble when trying to get Typescript projects running with ESM, it all feels very experimental to me. One solution might be to separate the frontend in a separate CommonJS module with its own package.json, but that feels clunky. Help welcome!

As a side note, I'd like to highlight nodejs/node#49441. This issue is annoying for our service classes and service tests, as they are all loaded dynamically. Basically, any syntax error in any of them will be caught by ESM loader, but you'll have no clue where the error is. ESLint was not clever enough to catch all the syntax errors either, the only reliable way I found to help me out was to get the ESM loader to tell me there was some error and then run Prettier in check mode... Whilst this issue is painful when migrating hundreds of files in one go, I suspect it won't be too much of a bother when we're modifying a few files in a normal PR.

[shields-ci]

	Warnings
⚠️	Lots 'o changes. Skipping diff-based checks.

	Messages
📖	Thanks for contributing to our documentation. We ❤️ our documentarians!
📖	✨ Thanks for your contribution to Shields, @PyvesB!

Generated by 🚫 dangerJS against bae618b

[lgtm-com]

This pull request introduces 1 alert when merging 3fb195c into 8f58817 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging c461cb1 into 8f58817 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging 4b23b29 into 8f58817 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[PyvesB]

We'll probably need to land #6365 first: ci/circleci: main and ci/circleci: services are failing on Node 12 due to syntax errors, whereas their Node 14 counterparts seem fine.

The LGTM alert should go away once merged.

[PyvesB]

To the best of my knowledge, this is now functional and is ready for review! 🍾

[paulmelnikow]

Hey @PyvesB, thanks for continuing to push this massive rock up the hill! Glad you've reached the top 🙌🏼

I've read a few of discussions about extensions in imports (including this one). I've not gone nearly as deeply down the rabbit hole as you have, though did have one question: could we avoid the explicit .js in all the import statements by renaming our files from .js to .mjs?

In general, I do think it is okay to merge this when it's working, and refine it later, as it's such a bear of a PR, though if we were to rename all the files, think that probably ought to happen before merging.

[paulmelnikow]

I've not read every service file, as once you get past ~ docsrs, the diffs are no longer rendered without clicking each file, but I tried to spot-checked the non-service files. When this is up to date with main, I'd be willing to give this another read locally since it'll let me page through the diff more easily. (I could also do that now if I knew which commit in main to compare to, though I tried and couldn't easily work out which one that is.

[chris48s]

I approached this with a list of things you might have forgotten or overlooked and you've thought of all of them. Beyond that I've had a good read over this and done some testing locally and on staging. I have one request for additional docs which shouldn't block the merge but would be useful to get done this side of your break if poss, but I don't have any requests for changes.
I think once Paul's comments are addressed (which don't look major) I'm on board with this 👍

[chris48s]

This isn't a blocker for merging this PR, but..

This is a workaround we will probably be stuck with for a while, but presumably at some point down the line we will be able to migrate the frontend from CommonJS to ESM too and then we can remove this.

I think it would be really useful to have an issue with a quick summary of

• How far did you get
• What problems did you encounter
• What are the blockers (is gatsby core the only package we know of with a problem, or do we know of other packages in the frontend tree that would also need an update to unblock us - was tsnode one?)
• If/when we can migrate the frontend to ESM:
  • What hacks can we remove?
  • Roughly what does that process look like?

Even if it is not 100% clear, I guess there is an extent to which that information currently exists in your head (or at least you have a clearer picture of this than anyone else) and it will be useful info at some point in future.

More broadly, I am so relieved you managed to find a way to avoid splitting the dependency tree in two 😌

[PyvesB]

Good point, will prepare a little issue with some of the stuff in my head before I head off on holidays. :)

[PyvesB]

Here we go: #6717

[lgtm-com]

This pull request introduces 1 alert when merging 9697ac1 into 23678fe - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[PyvesB]

I've read a few of discussions about extensions in imports (including this one). I've not gone nearly as deeply down the rabbit hole as you have, though did have one question: could we avoid the explicit .js in all the import statements by renaming our files from .js to .mjs?

The Node.js documentation does state the following:

A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified.

There is an experimental feature that allows automatic resolution of extensions, and I believe that's one of the things being talked about in the issue you linked, but if it isn't considered stable, I would not be keen on using it at this stage. :)

[paulmelnikow]

There is an experimental feature that allows automatic resolution of extensions, and I believe that's one of the things being talked about in the issue you linked, but if it isn't considered stable, I would not be keen on using it at this stage. :)

I am completely baffled that this "explicit" extension behavior is where Node has landed by default, and can't imagine this is going to be a popular option. Personally, I wouldn't mind using --experimental-specifier-resolution=node for a few months (since I think this will probably shake out) but I'm also okay with leaving the explicit extensions for now.

(It's clear that "type": "module" can be used in place of .mjs extensions.)

[lgtm-com]

This pull request introduces 1 alert when merging b4660d4 into 23678fe - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging 813cbc3 into 23678fe - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[PyvesB]

@paulmelnikow I pushed a last commit: we're no longer using require-hacker and I had forgotten to remove it from the dependencies. :)

[lgtm-com]

This pull request introduces 1 alert when merging bae618b into 23678fe - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class