NodeToolV0: allow reading versionSpec from file #15615
Conversation
|
thinking perhaps (maybe in a follow up PR) making the version spec parameter optional and then reading |
|
@anatolybolshakov: Could take a look at this PR, or recommend another reviewer? as per the docs I've semi-randomly picking you from the PR history :) |
|
@fowl2 thanks for contribution! Yes, let us take a look |
|
@anatolybolshakov gentle ping :) could you point me in the right direction? |
|
Note that it doesn't actually support |
|
You are indeed correct. This subset of the file format I think is still very useful. Completely matching nvm would be difficult short of actually using it - which isn't really feasible eg. because this needs to run on Windows. Perhaps the doc changes could be clearer, do you have a suggestion? |
|
Why isn't it feasible? nvm supports Windows just fine on WSL2, git bash, or cygwin (just not in powershell or cmd.exe). |
|
Yes technically possible, the best kind of correct ;) As I understand it, the agent runs in a "Windows Windows" ie. cmd.exe environment. Yes it would be possible to change this, but that would be an even bigger and wide reaching change that would have effects across all the actions, some of which are currently Windows only and likely to stay that way - eg the "run a Windows batch script" action. IMHO for if you need bug gif bug nvm, a new Linux/Cygwin only task that shells out to nvm could be created (or just shell out directly in a bash task). Or we could chip away at need functionality here, starting with the basics. :) |
|
Fair enough. It's pretty important tho that the caveats with reading nvmrc are well-communicated, especially since |
|
IMHO for, build pipelines, stability and reproducibility are most important. I'm struggling to imagine a node security vuln that would affect the code compiled with it, but I can easily see build breaks from patch releases. Maybe a failure of imagination on my part. I dream that one day the node version will be in the package lock and respected by |
|
That's a debate for another time, but suffice to say that reproducibility is not a universally desirable or achievable thing, in most ecosystems, but certainly not in the npm ecosystem. (and npm couldn't ever possibly address node versions; npm runs inside node) |
| return versionSpecInput; | ||
| } | ||
|
|
||
| const versionSpec = fs.readFileSync(versionSpecInput, { 'encoding': 'utf' }); |
There was a problem hiding this comment.
Could it make sense to add separate task input for version spec specified as file path - to avoid any confusion around it?
|
This PR is stale because it has been open for 180 days with no activity. Remove the stale label or comment on the PR otherwise this will be closed in 5 days |
|
Bump |
|
This PR is stale because it has been open for 180 days with no activity. Remove the stale label or comment on the PR otherwise this will be closed in 5 days |
|
I might take a look at this later this week |
|
This PR is stale because it has been open for 180 days with no activity. Remove the stale label or comment on the PR otherwise this will be closed in 5 days |
|
Looks like this was superseded by #16835 |
Task name: NodeToolV0
Description: allow reading versionSpec from file, for example a .nvmrc
Documentation changes required: Y
Added unit tests: Y
Attached related issue: #14094
Checklist: