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

feat: allow extracting of node+npm from package.json #807

Merged
merged 1 commit into from
Oct 14, 2023

Conversation

yoroshikun
Copy link
Contributor

Quick PR for allowing extracting the node and npm version from packages.json

The "pkgx" node version will always have priority, over "engines" use.

I also modified the tests to allow for more than one requirement to be added in one pass, let me know if this is not so good.

Possible improvements.

  • If pkgx is used it will take over and prevent npm requirement being added if it exists solely in engines. Maybe only check for if a node version is added to the deps.

Help

What is the correct way to test the dev function on real projects, do i need to install the build pkgx version and it should use that instead of needing to rebootstrap?

Copy link
Member

@mxcl mxcl left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you.

The "pkgx" node version will always have priority, over "engines" use.

I believe constraints are combined so any of them refine the others. Conflicts lead to error eg. one asks for node 16 the other node 18, but if one asks for ^16 and the other ~16.1 then the are combined (in this case the overlap is ~16.1).

I say “believe” rather than know because I'm pretty sure but didn't review the code and wrote it a while back (in mental terms).

If pkgx is used it will take over and prevent npm requirement being added if it exists solely in engines. Maybe only check for if a node version is added to the deps.

Not sure I understand you here.

What is the correct way to test the dev function on real projects, do i need to install the build pkgx version and it should use that instead of needing to rebootstrap?

deno task install

Will install your checkout to /usr/local/bin. dev uses that. Then just step into a dev-enabled directory.

@mxcl mxcl merged commit d8dbe80 into pkgxdev:main Oct 14, 2023
5 checks passed
@yoroshikun
Copy link
Contributor Author

If pkgx is used it will take over and prevent npm requirement being added if it exists solely in engines. Maybe only check for if a node version is added to the deps.

@mxcl examples are probs easier.

Currently the code works like this

{
  "engines": {
    "node": "~16.16.1",
    "npm": "~9.7.1"
  }
}

Resolved, nodejs.org~16.16.1,npmjs.com~9.7.1

{
  "pkgx": "zlib@1.1.1",
  "engines": {
    "node": "~16.16.1",
    "npm": "~9.7.1"
  }
}

Resolved zlib@1.1.1 (only)

since pkgx takes priority.

Based on this I plan to make a follow up PR (or few) to make this a bit more robust at choosing and combining correctly in the packagejson helper.

@mxcl
Copy link
Member

mxcl commented Oct 17, 2023

Based on how we are handling other such situations we should not remove the computed engines if there is a pkgx node. If the user specifies (eg) node in both places then we attempt to consolidate them (this is done for you automatically), if they cannot be combined into a single range it throws.

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 this pull request may close these issues.

None yet

2 participants