Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1118 +/- ##
==========================================
- Coverage 87.21% 80.32% -6.90%
==========================================
Files 137 137
Lines 12593 12615 +22
==========================================
- Hits 10983 10133 -850
- Misses 1610 2482 +872 ☔ View full report in Codecov by Sentry. |
src/plugins/core/node.rs
Outdated
| self.install_npm_shim(&ctx.tv)?; | ||
| self.test_npm(ctx.config, &ctx.tv, &ctx.pr)?; | ||
| self.install_default_packages(ctx.config, &ctx.tv, &ctx.pr)?; | ||
| if *env::RTX_NODE_COREPACK_ENABLE && self.corepack_path(&ctx.tv).exists() { |
There was a problem hiding this comment.
I think we should check the version and if it's before corepack was released (not sure when that was) we should not try to enable it even if they set this to true
There was a problem hiding this comment.
Sounds good. It was introduced in two minors—v14.19.0 and v16.9.0—so I'll add a method to compare with versions that ship with corepack. Recommend I leave the exists() check in place even with the version check?
There was a problem hiding this comment.
oh sorry I didn't notice that, I think that check is sufficient
|
in general looks good |
2 participants
This is more to convey an idea than anything else—I'm not a rusty kind of person.
I use corepack—which has shipped with node since v14 and v16—since I bounce between a bunch of different nodejs codebases that use different package managers. Love
rtxbut I have to remember to reenable corepack every time I install a new version.So this is my attempt at running
corepack enableautomatically after installing a new node version.What do you think? Figured this might convey what I was thinking better than opening a discussion not necessarily because I think this code should be merged. 😄