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

feat(scripts): Create script to help with CJS -> ESM migration #8197

Merged
merged 4 commits into from
Jun 12, 2024

Conversation

cpcallen
Copy link
Contributor

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
Contributor 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
Collaborator

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
Contributor Author

OK, this is now ready for review.

Copy link
Collaborator

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

scripts/migration/cjs2esm Show resolved Hide resolved
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
Collaborator

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
Contributor 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.

scripts/migration/cjs2esm Outdated Show resolved Hide resolved
Comment on lines +48 to +50
if (moduleName[0] === '.') {
// Relative path. Could check and add '.mjs' suffix if desired.
}
Copy link
Collaborator

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
Contributor 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.

scripts/migration/cjs2esm Outdated Show resolved Hide resolved
scripts/migration/cjs2esm Outdated Show resolved Hide resolved
scripts/migration/cjs2esm Outdated Show resolved Hide resolved
scripts/migration/cjs2esm Outdated Show resolved Hide resolved
scripts/migration/cjs2esm Outdated Show resolved Hide resolved
scripts/migration/cjs2esm Show resolved Hide resolved
@cpcallen cpcallen merged commit 5a9a31a into google:develop Jun 12, 2024
6 checks passed
@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