Migrate to ESM #324
Migrate to ESM #324
Conversation
| path && logger.debug(`Loading config from ${path}`); | ||
| if (path) { | ||
| logger.debug(`Loading config from ${path}`); | ||
| } |
There was a problem hiding this comment.
Had to update these to satisfy new eslint rules
| }, | ||
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
I am admittedly not 100% sure why this one was necessary, but it's just a test so I think that's ok. Claude really struggled with getting mocking to work to test behavior on windows
There was a problem hiding this comment.
Wait how did you test this on Windows?
| * @returns {Promise} - Returns _default_ exported content if ESM, or exported module content if CJS. | ||
| */ | ||
| async function dynamicImport(filePath: string): Promise<any> { | ||
| if (semver.gte(process.version, '13.2.0')) { |
There was a problem hiding this comment.
We should test this but I don't think any of this code was necessary because we no longer support this node version
There was a problem hiding this comment.
Yeah this is ok to remove if this is going to be a major release anyways
| } | ||
| }; | ||
|
|
||
| async close(): Promise<void> { |
There was a problem hiding this comment.
Adding this helper made the PortManagerServer a lot easier to test (failing tests here were why I started this process in the first place)
camden11
requested review from
brandenrodgers,
chiragchadha1 and
joe-yeager
as code owners
| "yargs": "^17.7.2" | ||
| }, | ||
| "exports": { | ||
| "./*": "./lib/*.js", |
There was a problem hiding this comment.
Should we add a line here for ./*/*? Or is that what you're doing in a more targeted way with the new cms export?
There was a problem hiding this comment.
I figured it'd be better to be explicit about it (we do the same thing with the config dir) but open to changing it too
Description and Context
This PR migrates local-dev-lib to use ESM rather than CommonJS. It does a few other things to make this possible
The vast majority of the changes in this PR are just updates to import paths (for ESM) and changes from vite to jest. I tried to call out anything beyond that in the comments
TODO
Who to Notify
@brandenrodgers @chiragchadha1 @joe-yeager