commit-analyzer icon indicating copy to clipboard operation
commit-analyzer copied to clipboard

refactor(esm): converted the package to esm

Open travi opened this issue 3 years ago • 6 comments

BREAKING CHANGE: @semantic-release/commit-analyzer is now a native ES Module. It has named exports for each plugin hook (analyzeCommits)

fixes #296

travi avatar Dec 16 '21 19:12 travi

i've spent more time that i'd like to admit trying to understand several instances of this error:

 Error {
    code: 'ERR_REQUIRE_ESM',
    message: `require() of ES Module /Users/path/to/semantic-release/commit-analyzer/test/fixtures/release-rules.js from /Users/path/to/semantic-release/commit-analyzer/noop.js not supported.␊
    Instead change the require of release-rules.js in /Users/path/to/semantic-release/commit-analyzer/noop.js to a dynamic import() which is available in all CommonJS modules.`,
  }

and where noop.js was even coming from. in the end, i think it is related to https://github.com/sindresorhus/import-from/issues/9 and https://github.com/semantic-release/commit-analyzer/blob/7c693d913969b7c9c17251fc8b2213eb9dcfb386/lib/load-release-rules.js#L24

we may need to consider an alternative approach with this. any thoughts come to mind @gr2m?

travi avatar Dec 16 '21 19:12 travi

Not out of hand, I don't think I used importFrom in any other ESM project. Maybe we can use another approach that doesn't need import-from?

gr2m avatar Dec 17 '21 18:12 gr2m

Maybe we can use another approach that doesn't need import-from?

yeah, i think thats what i'm looking for ideas around. i havent thought too far about it yet, but i think we may need to come up with alternative ideas for this piece.

travi avatar Dec 17 '21 18:12 travi

May I ask for status of this? I just switched an App to ESM and I found semantic-release is not working, which probably depends on this PR to be merged (among others). Since I have this thing fresh, I may add some value to any roadblock, if you can provide some status for me.

dgilperez avatar Jun 06 '22 15:06 dgilperez

I just switched an App to ESM and I found semantic-release is not working, which probably depends on this PR to be merged

semantic-release runs as its own executable, so the type of your project should have no impact on how semantic-release executes. if you have a .releaserc.js to configure semantic-release, that is the one exception and you may need to convert that file to match your project.

travi avatar Jun 06 '22 19:06 travi

Thanks a lot! I managed to have it working by setting the releaserc.js file properly - thanks!

El lun., 6 jun. 2022 21:03, Matt Travi @.***> escribió:

I just switched an App to ESM and I found semantic-release is not working, which probably depends on this PR to be merged

semantic-release runs as its own executable, so the type of your project should have no impact on how semantic-release executes. if you have a .releaserc.js to configure semantic-release, that is the one exception and you may need to convert that file to match your project.

— Reply to this email directly, view it on GitHub https://github.com/semantic-release/commit-analyzer/pull/297#issuecomment-1147790178, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACATDW2A4SQGBMPUIKF37TVNZDR3ANCNFSM5KHEJC3A . You are receiving this because you commented.Message ID: @.***>

dgilperez avatar Jun 06 '22 19:06 dgilperez

i've spent more time that i'd like to admit trying to understand several instances of this error:

 Error {
    code: 'ERR_REQUIRE_ESM',
    message: `require() of ES Module /Users/path/to/semantic-release/commit-analyzer/test/fixtures/release-rules.js from /Users/path/to/semantic-release/commit-analyzer/noop.js not supported.␊
    Instead change the require of release-rules.js in /Users/path/to/semantic-release/commit-analyzer/noop.js to a dynamic import() which is available in all CommonJS modules.`,
  }

and where noop.js was even coming from. in the end, i think it is related to sindresorhus/import-from#9 and

https://github.com/semantic-release/commit-analyzer/blob/7c693d913969b7c9c17251fc8b2213eb9dcfb386/lib/load-release-rules.js#L24

hey @travi :wave: I see the same code has been released in v11.0.0 of @semantic-release/release-notes-generator (about a month ago), so I'm wondering if this is still a blocking point, since versions of import-from are both set to ^4.0.0 and the importFrom function is used in the same exact way?

And if there's still an issue here, could that imply the same issue is present in v11 of @semantic-release/release-notes-generator?

sheerlox avatar May 26 '23 13:05 sheerlox

to be honest, i haven't had the chance to revisit the conversion of this plugin, so i'm not sure if the things that we've learned from converting other packages would apply in a way that would get us past this concern in this context.

And if there's still an issue here, could that imply the same issue is present in v11 of @semantic-release/release-notes-generator?

are you encountering a problem in the release notes generator that you think could be related to this?

travi avatar May 26 '23 14:05 travi

are you encountering a problem in the release notes generator that you think could be related to this?

not at all! I came to this issue because I was trying to update some dependencies on my conventional-changelog preset (e.g. https://github.com/insurgent-lab/conventional-changelog-preset/pull/8), then figured out that if I transitioned the preset to ESM, @semantic-release/commit-analyzer wouldn't be able to load it anymore, right?

this is not a big deal though, so I'll just monitor this PR and transition the preset to ESM when it becomes possible, no pressure :wink:

sheerlox avatar May 26 '23 14:05 sheerlox

then figured out that if I transitioned the preset to ESM, @semantic-release/commit-analyzer wouldn't be able to load it anymore, right?

this is very possible, and quite honestly likely. i dont have the full picture in my head at the moment, but from brief consideration this morning, i do believe you are correct. i think we are in an ok state with release-notes-generator because existing presets have not yet converted to esm.

you are highlighting that we are at risk of breakage for folks if a preset does convert since i didnt come across the consideration that i caught above in the conversion of the release-notes-generator. i really appreciate that you dug deep enough to figure this out while considering the conversion of your preset, but especially appreciate that you reached out with the information to have a conversation with us about it.

travi avatar May 26 '23 14:05 travi

the other thing that i think is worth calling out as a result of this information, as it relates to moving the effort forward for this package, i think we could consider this detail to not be a blocker for converting this plugin. that choice would leave us in the situation where we would be unable to load presets that have been converted to esm, but we are already in that situation with release-notes-generator. while we may want to enable loading of esm presets, it could make sense to follow up with a separate effort for enabling that sometime after we convert this package to esm.

travi avatar May 26 '23 14:05 travi

it's a pleasure really, been using semantic-release for more than 4 years, so many thanks to you guys for creating and maintaining this awesome set of tools!

P.S: I'd love to offer to contribute on this issue, but right now I already have a lot on my mind. I might come back to it in a while if it is still hanging :wink:

sheerlox avatar May 26 '23 14:05 sheerlox

it could make sense to follow up with a separate effort for enabling that sometime after we convert this package to esm.

that definitely makes sense to me!

sheerlox avatar May 26 '23 14:05 sheerlox