Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkgx reads .nvmrc and can't parse lts/iron or ^20.0.0 #1051

Open
chmac opened this issue Nov 25, 2024 · 5 comments · May be fixed by #1052
Open

pkgx reads .nvmrc and can't parse lts/iron or ^20.0.0 #1051

chmac opened this issue Nov 25, 2024 · 5 comments · May be fixed by #1052

Comments

@chmac
Copy link

chmac commented Nov 25, 2024

$ dev
× unexpected error invalid semver range: @lts/iron

Here's the situation, we have lts/iron in .nvmrc. I tried adding .node-version and pkgx: to package.json but I still get the error. Then I tried changing .nvmrc to ^20.0.0 and I get the following error:

$ dev
× unexpected error invalid semver range: @^20.0.0

Next try gave:

$ dev
× unexpected error invalid semver range: @>20.0.0 <21.0.0

There's no mention of .nvmrc in the dev docs. Also, the error message that this is an invalid semver range is a little misleading (I think). It seems that it only accepts exact versions rather than ranges. Or maybe I misunderstood something.

It would be great if there was a clear precedence to pkgx files. I also tried adding pkgx.yml but that generates errors if both .nvmrc and pkgx.yml don't have the same version.

Edit: It seems like a bug that pkgx parses .nvmrc looking for a semver range, given that nvm itself supports other version specifiers. It seems like a separate bug that ^20.0.0 is flagged as an invalid semver range.

@jhheider
Copy link
Contributor

jhheider commented Nov 25, 2024

yes, the problem is here:

pkgx/src/utils/devenv.ts

Lines 145 to 150 in f28d931

async function version_file(path: Path, project: string) {
let s = (await path.read()).trim()
if (s.startsWith('v')) s = s.slice(1) // v prefix has no effect but is allowed
s = `${project}@${s}`
pkgs.push(utils.pkg.parse(s))
}

unlike other version files, we treat .nvmrc and .node-version as only capable of having a since version specifier. likely a fix like this would work:

  async function version_file(path: Path, project: string) {
    let s = (await path.read()).trim()
    if (s.startsWith('v')) s = s.slice(1)  // v prefix has no effect but is allowed
    if (s.match(/^[0-9]/)) s = `@${s}`  // bare numbers are single versions
    s = `${project}${s}`
    pkgs.push(utils.pkg.parse(s))
  }

jhheider added a commit that referenced this issue Nov 25, 2024
@jhheider jhheider linked a pull request Nov 25, 2024 that will close this issue
jhheider added a commit that referenced this issue Nov 25, 2024
jhheider added a commit that referenced this issue Nov 25, 2024
@chmac
Copy link
Author

chmac commented Nov 26, 2024

@jhheider Thanks for the quick follow up. What will happen with this patch if a .nvmrc file contains lts/iron?

My expectation would be that pkgx ignores third party files which it can't parse, but that's not what I observed. When it couldn't make sense of lts/iron it seemed to break my ability to automatically make the correct node version available.

I realise there's a few different topics in this issue, if it would be helpful for me to split them into separate issues just let me know, happy to do that. And thanks for pkgx, it's really awesome, I'm looking forward to making it our go-to tool to get new developers setup on our environment.

@jhheider
Copy link
Contributor

I haven't altered that behavior. The obvious answer should be that it ignores the invalid content.

Question: is there a benefit I don't know to using named releases instead of versions? Or does nvm just resolve them (via some published method) into semver strings? Because, until we do ignore data we can't use, that seems like the correct way to use the tools together.

@chmac
Copy link
Author

chmac commented Nov 26, 2024

I'm not sure how nvm does the resolution internally. The lts releases all have official names, which is where the format comes from. It's described in this section of the nvm docs. You can see the node version code names here. I would guess that lts/iron is equivalent to ^20.0.0 in that it's the latest release in the 20.* line.

Let me know if I can do anything else to help out.

@jhheider
Copy link
Contributor

I think supporting nodejs release code names is probably too moving a target to be wise, but ignoring them should be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants