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

Fix: correct node version parse #884

Merged
merged 9 commits into from
May 15, 2023

Conversation

Miloas
Copy link
Contributor

@Miloas Miloas commented May 11, 2023

This PR:
fix #858

#858 describe our current implementation cant parse ^14.10.3 correctly, it will return the default node version, but it should return 14.

In real world node-semver is more complicated. You can check how npm handle this https://github.com/npm/node-semver

Fortunately, we have a great crate can do same thing https://github.com/felipesere/node-semver-rs

How I think the original implementation?

inputs (one of):

  1. package.json -> engines -> node (node semver)
  2. .nvmrc (may prefixed v)
  3. NODE_VERSION (node semver)

outputs:
node version that statisfied the inputs version (one of):

  1. 14
  2. 16
  3. 18

@Miloas Miloas changed the title fix: correct node version parse WIP: Fix: correct node version parse May 11, 2023
@Miloas Miloas force-pushed the fix/node-version-parse branch 4 times, most recently from 464302f to 2871cd2 Compare May 12, 2023 11:57
@Miloas Miloas changed the title WIP: Fix: correct node version parse Fix: correct node version parse May 12, 2023
@Miloas Miloas force-pushed the fix/node-version-parse branch from 2871cd2 to 7aefcb7 Compare May 12, 2023 12:02
@Miloas
Copy link
Contributor Author

Miloas commented May 12, 2023

@half0wl please would you take a look this pr?

@Miloas
Copy link
Contributor Author

Miloas commented May 12, 2023

BTW, I found every languages may have their own semver rules (for example, rust semver is different of node semver, most of languages use standard semver), so I cant port it to other languages. But I think other provider may have same issue.

src/providers/node/mod.rs Outdated Show resolved Hide resolved
src/providers/node/mod.rs Outdated Show resolved Hide resolved
@coffee-cup coffee-cup added the release/minor Author minor release label May 12, 2023
@Miloas Miloas force-pushed the fix/node-version-parse branch from 0207c5b to 82f2b06 Compare May 12, 2023 19:42
@Miloas Miloas force-pushed the fix/node-version-parse branch from 82f2b06 to 2c022f2 Compare May 12, 2023 19:47
@Miloas
Copy link
Contributor Author

Miloas commented May 12, 2023

Rust clippy: "you looks not smart" 😢

Copy link
Member

@half0wl half0wl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

BTW, I found every languages may have their own semver rules (for example, rust semver is different of node semver, most of languages use standard semver), so I cant port it to other languages. But I think other provider may have same issue.

Yeah, good call pulling in node-semver to deal with this! Happy to see the regex gone ;-)

@half0wl half0wl merged commit 514f7a9 into railwayapp:main May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/minor Author minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node provider fails to detect caret-prefixed versions
3 participants