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

Converting from commonjs to esm #1210

Merged
merged 28 commits into from
May 18, 2022
Merged

Converting from commonjs to esm #1210

merged 28 commits into from
May 18, 2022

Conversation

scalvert
Copy link
Contributor

@scalvert scalvert commented May 10, 2022

Relates to #1201

Description

This is a massive refactor that converts Checkup from cjs to esm. The intersection of esm and TypeScript is a path that's not well trodden, and as such has taken some time to figure out and get right. While there was a concerted effort to decouple the scope of changes required to move over to esm, some of the changes were unavoidably combined together in order to maintain some semblance of cohesion within the codebase in order to feasibly perform this migration.

Changes include:

  • Moving all packages over to use "type": "module"
  • Removing all variants of require and import * as statements
  • Converting __dirname to the esm equivalent (using a public package)
  • Converting the test infra to use vitest vs. jest. While this change wasn't 100% necessary, converting to use vitest for tests has afforded us the ability to greatly simplify the test infrastructure by avoiding the use of jest + ts-jest, both of which aren't esm-first (which vitest is)
  • Fixing all linting errors.

Outstanding changes that will not be included in this refactor are:

  • Upgrading add dependencies to their esm variants (a number of packages have esm published variants, but an attempt was made to reduce the scope of changes and add those incrementally before the 2.0 release)
  • Re-introducing eslint-plugin-imports to correctly work with esm + relative file paths. I could not get the configuration correct in order to ensure this was working correctly, so for now it's disabled. This will be fixed in a follow-up PR.

Remaining things TODO in this PR:

  • Remove all vestiges of jest from the repo (may require a new version of jest-sarif that is compatible with vitest, currently in progress)
  • Conversion to npm over yarn. Currently the CI jobs are failing due to mismatched engine versions (the repo currently has references to v1.4.2 packages of Checkup, which use an older version of node. While we can temporarily ignore engine mismatches with --ignore-engines, yarn run won't allow invocations with packages with invalid version numbers, hence blocking CI from passing). Converting to npm may fix this.

@scalvert scalvert requested a review from zhanwang626 May 18, 2022 15:42
@scalvert scalvert marked this pull request as ready for review May 18, 2022 15:43
@scalvert
Copy link
Contributor Author

Going to pause the npm migration as there's a type resolution conflict that occurs within npm's overrides that doesn't occur in yarn's resolutions. Until we can normalize the version numbers across packages, we should keep on yarn.

@scalvert scalvert added this to the v2.0 milestone May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant