Skip to content

Conversation

@cpcallen
Copy link
Collaborator

The basics

The details

Proposed Changes

Create script to help with CJS -> ESM migration.

Reason for Changes

We need to finish migrating tests to ESM as the latest version of chai no longer supports loading via require()—see PR #8092.

Test Coverage

Some very cursory manual testing of this script has been done. No changes to testing of Blockly anticipated.

Additional Information

This is based on js2ts, but considerably simplified since we no longer need to deal with goog.module IDs.

This is based on js2ts, but considerably simplified since we
no longer need to deal with goog.module IDs.
@cpcallen cpcallen added the PR: feature Adds a feature label Jun 10, 2024
@cpcallen cpcallen requested a review from a team as a code owner June 10, 2024 07:23
@cpcallen cpcallen requested a review from BeksOmega June 10, 2024 07:23
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Jun 10, 2024
@cpcallen cpcallen marked this pull request as draft June 10, 2024 14:52
@cpcallen
Copy link
Collaborator Author

Marking this draft as I want to make a few changes to deal with a few common syntax styles that we use in our CJS modules that we didn't seem to use (or at least: not frequently) in our goog.modules—notably module.exports = {foo, bar, baz};.

@BeksOmega
Copy link
Contributor

Sweet sweet, please send me a ping when this reopens b/c GitHub doesn't notify!

Support (optionally renaming) assignments to module.exports, in
addition to the existing support for simple exports property
assignmenets.
@cpcallen cpcallen marked this pull request as ready for review June 10, 2024 15:38
@cpcallen
Copy link
Collaborator Author

OK, this is now ready for review.

Copy link
Contributor

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No functional comments! Just some Qs, some docs, and some spellings.

Comment on lines 16 to 24
const requireRE =
/(?:const\s+(?:([$\w]+)|(\{[^}]*\}))\s+=\s+)?require\('([^']+)'\);/g;

/** RegExp matching key: value pairs in destructuring assignments. */
const keyValueRE = /([$\w]+)\s*:\s*([$\w]+)\s*(?=,|})/g;

/** Prefix for RegExp matching a top-level declaration. */
const declPrefix =
'(?:const|let|var|(?:async\\s+)?function(?:\\s+\\*)?|class)';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just gonna trust this is correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I could write …|function|async function|function *|async function *|…

Though actually I just noticed an error: the space between function and * in generator decls is optional.

Comment on lines +48 to +50
if (moduleName[0] === '.') {
// Relative path. Could check and add '.mjs' suffix if desired.
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mean to be empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There is a bit of optional code that I might write to save having to fix up import paths manually—the only difficulty is that sometimes the imported filename will be an .mjs and other times it will just be .js, and not even testing file existence can necessarily distinguish because the file in question might be mid-conversion by the same invocation of this tool. So I left this as a placeholder for the moment.

@cpcallen cpcallen merged commit 5a9a31a into RaspberryPiFoundation:develop Jun 12, 2024
@cpcallen cpcallen deleted the feat/cjs2esm branch June 12, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: feature Adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants