POC, do not merge: ESM update and Vitest switchover#98
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
This is a clean migration from CommonJS to ESM and from Jest to Vitest. The changes are well-structured and consistent across all files. One minor issue exists in the fingerprint test where a commandPath variable is declared but never used after the refactor, and there's a subtle ordering issue in verify.js where const debug is declared before an import statement.
📝 2 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🤖 PR Reviewer
This is a clean migration from CommonJS/Jest to ESM/Vitest. The changes are well-structured and consistent across all files. Two previously raised issues remain: the commandPath variable in fingerprint.test.js is still declared but only used for dynamic imports in beforeAll blocks (not at the top level where TheCommand is now imported directly), and in generate.js the const debug assignment is placed before a blank line after imports but is still mixed in without a blank line separator from the imports. Overall the migration is solid.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
This diff migrates the project from CommonJS/Jest to ESM/Vitest cleanly. The re-raised issues about commandPath placement and the missing blank line before const debug in generate.js are still present. No new significant issues found — the module mocking pattern in fingerprint.test.js is a reasonable workaround for vitest's lack of isolateModules, and the overall migration is correct.
🔄 2 re-raised suggestion(s) from previous review
Findings (line numbers not in diff):
test/commands/certificate/fingerprint.test.js(line 15): [Re-raised]commandPathis declared at the top level butTheCommandis imported dynamically insidebeforeAllblocks. The top-level declaration is misleading — it suggestsTheCommandwas loaded via this path at module load time, but it wasn't. Consider adding a clarifying comment.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
|
/review |
There was a problem hiding this comment.
🤖 PR Reviewer
The migration from CommonJS to ESM and from Jest to Vitest is clean and well-executed. The previous suggestion about the missing blank line in generate.js has been addressed — the blank line is now present between the last import and const debug. No significant issues remain.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
purplecabbage
left a comment
purplecabbage
left a comment
There was a problem hiding this comment.
just a nit about left over commented out // requires()
There was a problem hiding this comment.
🤖 PR Reviewer
This PR migrates the project from CommonJS to ESM and from Jest to Vitest. The changes are well-structured and consistent across all files. The migration approach for handling module isolation in fingerprint tests is reasonable given Vitest's limitations with isolateModules. One minor issue exists in the .gitignore file.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| junit.xml | ||
| package-lock.json | ||
| .claude | ||
| .cursor/ No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file
| .cursor/ | |
| .cursor/ |
There was a problem hiding this comment.
🤖 PR Reviewer
This diff migrates the project from CommonJS to ESM and from Jest to Vitest. The changes are well-structured and consistent. The previous .gitignore newline issue has been resolved. No significant issues found.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
This PR migrates the project from CommonJS to ESM and from Jest to Vitest. The changes are consistent and well-structured. One minor issue: the copyright year in the new test files says '2026', which appears to be a typo.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
2 participants
Description
POC: change this plugin to an ESM module.
Related Issue
https://jira.corp.adobe.com/browse/ACNA-4658
Motivation and Context
Test how feasible upgrading plugins in aio-cli (entirely CommonJS) to ESM would be. Many of our deprecated dependencies are ESM-only if you want the non-deprecated versions, and we do not want to lazy load all our dependencies.
How Has This Been Tested?
Unit tests: made sure they run with 100% coverage (including adding a few for index.js).
End-to-end: linked plugin to local aio-cli and ran all three
aio certificatecommands without issues.Possible issue: The ESM version does print the warning "@adobe/aio-cli-plugin-certificate is a linked ESM module and cannot be auto-transpiled. Existing compiled source will be used instead." The plugin does not use Typescript and seems to function fine in spite of this warning, but it should still be noted.
Screenshots (if appropriate):
Types of changes
Checklist: