Closed sheerlox closed 11 months ago
these are great PRs and highlight the duplication we have for loading across these two plugins (which i'd like to resolve at some point).
I wont be able to take a close look for a few days, but should be able to take a look friday at the latest. the one thing that i'm wondering without a deeper look is if there would be a way to cover the full loading of a cjs preset and esm preset with an integration test. it may be easier to do this in the core repo, but i wanted to put the thought out there in case you'd already considered options about this before i can look deeper.
these are great PRs and highlight the duplication we have for loading across these two plugins (which i'd like to resolve at some point).
are you thinking about creating a separate @semantic-release/conventional-preset-loader
package to centralize the module-loader.js
file into a single repository?
the one thing that i'm wondering without a deeper look is if there would be a way to cover the full loading of a cjs preset and esm preset with an integration test. it may be easier to do this in the core repo, but i wanted to put the thought out there in case you'd already considered options about this before i can look deeper.
I could work on a PR on the core repo to add that kind of test if you confirm that's the best option, let me know!
I wont be able to take a close look for a few days, but should be able to take a look friday at the latest.
not in a hurry, I'm just glad I could find some time to work on this since we talked about it a few months ago :smile:
While doing research today, I found a "corner" case I didn't test that should fail with this PR.
The issue is with loading a module from the parent module node_modules
: "dependencies (without an explicit path) for a given module are searched for relative to the module loading them" (source).
That means the current code probably won't be able to load conventional-commit presets installed along semantic-release
.
I didn't run into this issue while testing, since I'm using a relative path to my local preset, and test suites cannot detect that bug since they are using their own node_modules
folder.
I'm working on a solution.
are you thinking about creating a separate
@semantic-release/conventional-preset-loader
package to centralize themodule-loader.js
file into a single repository?
yeah, i'm definitely leaning in this direction, but that would be a future step. no need to worry about this at this point other than maybe trying to be careful to keep the two current implementations as similar as possible and call out where they differ and how necessary any differences are.
I could work on a PR on the core repo to add that kind of test if you confirm that's the best option, let me know!
i think that is the easiest place to add them today, but it has the downside of not catching issues locally within this package before publishing. with the additional use case you mentioned most recently, that highlights even further that an integration test that truly loaded the configs could be a helpful feedback loop. i wont hold up getting this merged based on existence of that type of test, but would be open to the addition if you think it helps give you feedback that would give you more confidence with the solution. if you decide it is worth it, let me know how i can be helpful
after encountering the error with importing a module from multiple levels above, I realized the task at hand was a bit more complex than expected and that it might be useful to others, so I decided to port import-from
to support importing ES modules (by using import
instead of require
) while maintaining the same module resolution strategy require
uses. I introduced only one breaking change, which is the need for await
(since import
is asynchronous).
the package also has the ability to load files regardless of their extension (.js
, .mjs
, .cjs
), note they still need to have the correct type depending on the module
attribute of their closest package.json
(i.e. importing a JS file containing a CJS module while "type": "module"
is set will throw an error). this was necessary since with resolve
the file extension was optional.
I've tested this version of the PRs with 4 different configurations: CJS /or/ ESM preset /and/ relative path /or/ npm module specifier.
source for the new package is available at sheerlox/import-from-esm, please let me know what you think of this solution.
creating a @semantic-release/conventional-preset-loader
package is a good call: while commit-analyzer/load-release-rules.js
is on its own, the code and tests between changelog-notes-generator/load-changelog-config.js
and commit-analyzer/load-parser-config.js
are the same, with writerOpts
added in load-changelog-config.js
(so commit-analyzer
could use changelog-notes-generator
's implementation. grouping everything would greatly improve consistency and testing, and reduce duplication.
i wont hold up getting this merged based on existence of that type of test, but would be open to the addition if you think it helps give you feedback that would give you more confidence with the solution.
I think that type of test would be a great addition, and indeed would add to my confidence for this PR. but this is going to take time, and since I've been kicking issues down the road for weeks, I'd prefer if you could raise an issue to remember that needs to be done if that's okay for you. I've extensively tested the solution I'm proposing and I'm feeling confident about it.
I always monitor my GH notifications closely, and I'll free some time right away if work needs to be done either on import-from-esm
or @semantic-release
packages after merging these PRs.
Note: I've made significant efforts to get the best ossf/scorecard
score possible for import-from-esm
, and the package is currently getting a score of 8.2
.
The two scopes I cannot improve on for now are Code-Review
and Maintained
, since I'm the only maintainer and the repository is only 9 days old.
If someone from @semantic-release would like to be invited as a maintainer of the repository, please just let me know and consider it done.
sorry that i've been slow to get back to these. i dont think i'm going to get a chance today, but these are still on my radar
source for the new package is available at sheerlox/import-from-esm, please let me know what you think of this solution.
i like the solution and appreciate that you've mentioned it in the thread for the original project.
there is one detail about the package implementation that i'm curious about. it looks like you've vendored a manually tree-shaken version of import-meta-resolve. can you help me understand the benefit of vendoring it that way? the downside that stands out to me is that pulling in a potential vulnerability patch would now be more manual than getting an automated update PR from dependabot or renovate (and consumers would also not get a notification as easily that a vulnerability exists).
There are two reasons for this choice:
packageResolve
) is not exposed by import-meta-resolve
import-meta-resolve
, it also helps to reduce package sizeI see two different ways of resolving the issues you mentioned:
import-meta-resolve
's maintainer if exposing packageResolve
would be okay, propose a PR, and use the package directly. That would increase the import-from-esm
package size by 20,2 kB
, but I guess that's not a big deal given the security benefits you mentionedpackageResolve
from import-meta-resolve
The first solution indeed seems to be the most straightforward and durable/stable solution. Please let me know your thoughts on this, and I'll raise an issue ASAP to discuss that with the maintainer.
- the function I was interested in (
packageResolve
) is not exposed byimport-meta-resolve
that makes sense. i overlooked this detail. i dont want a suggestion to improve that to hold up these PRs. if you could update the PRs with mainline once more to resolve conflicts, i'm good with getting these merged. they are great contributions and get us past what i think is our last ESM hurdle in core functionality.
- ask the
import-meta-resolve
's maintainer if exposingpackageResolve
would be okay, propose a PR, and use the package directly. That would increase theimport-from-esm
package size by20,2 kB
, but I guess that's not a big deal given the security benefits you mentioned
this seems worthwhile to at least ask about. i wouldnt be concerned about the additional size, honestly. that is such a small difference and being more easily aware of potential vulnerabilities and general improvements greatly outweigh the size benefit, imho. if the answer is no for that package, it could make sense to continue to vendor, but would be interesting to hear their answer.
Raised https://github.com/wooorm/import-meta-resolve/issues/18 to ask if exporting the packageResolve
function would be possible, I'll follow up on that and release a new version whenever possible if the maintainer agrees on the changes. Otherwise, I'll find an automation solution.
:tada: This PR is included in version 12.1.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
Turns out exporting the packageResolve
function wasn't necessary, I successfully replaced the vendored version of import-meta-resolve
with a package dependency and used their moduleResolve
function in https://github.com/sheerlox/import-from-esm/pull/35. I've added extensive tests beforehand in https://github.com/sheerlox/import-from-esm/pull/34.
This is going to be released as v1.1.0
, as the package now supports subpath imports, and the changes are not breaking.
This PR adds support for loading pure ESM presets. This was not previously possible because
import-from
usescreateRequire
under the hood.In order to replace it, I've created an ES module loader that abstracts the preset loading logic for
semantic-release
repos (first tries to hitnode_modules
, then if not found tries to hit the relative path from the current working directory).I've tested these changes locally on the https://github.com/insurgent-lab/conventional-changelog-preset repo (which is a preset but also uses its current version for releasing with
semantic-release
), both with the current latest version and the ESM version (which you can find in therefactor/esm
branch since it's probably the only conventional-changelog ESM preset at this time).Related
cc @travi :wink: