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

chore: align expected node engine with volta #7998

Merged
merged 5 commits into from
Oct 24, 2023

Conversation

alisonailea
Copy link
Contributor

Related Issue: n/a

Summary

Trying to build the Calcite Design System packages with node 16 fails. It requires at least node 18.

Trying to build the Calcite Design System packages with node 16 fails. It requires at least node 18.
@alisonailea alisonailea added the tooling Issues relating to build system fixes or improvements. label Oct 12, 2023
@github-actions github-actions bot added the chore Issues with changes that don't modify src or test files. label Oct 12, 2023
package.json Outdated
@@ -111,7 +111,7 @@
},
"license": "SEE LICENSE.md",
"engines": {
"node": ">=16.0.0"
"node": ">=18.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be >=18.18.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anything over 18 will work. I wanted to keep it flexible but I can update

Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

Specifying engines displays warnings for our users if they install CC with a node version that is not in the specified range. I'm pretty sure CC will still work on node v16 so we shouldn't throw warnings when people try to use it. Volta is what we use to specify our development version of Node.

ref: https://docs.npmjs.com/cli/v6/configuring-npm/package-json#engines

@alisonailea
Copy link
Contributor Author

Specifying engines displays warnings for our users if they install CC with a node version that is not in the specified range. I'm pretty sure CC will still work on node v16 so we shouldn't throw warnings when people try to use it. Volta is what we use to specify our development version of Node.

ref: https://docs.npmjs.com/cli/v6/configuring-npm/package-json#engines

That only applies to the engine listed in the calcite-components package, which doesn't currently have an engine listed at all. In the CONTRIBUTING docs it does not say you must use Volta but rather directs you to use the major version of node listed in the package.json (implying the engine key).

@@ -87,7 +87,7 @@
"@stencil/state-tunnel": "1.0.1"
},
"license": "SEE LICENSE.md",
"volta": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm removing volta at this level because a calcite-components consumer will not have the context of the monorepo to reference the volta node version.

Copy link
Member

Choose a reason for hiding this comment

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

The volta version is not for the calcite consumer. It is for automatically using the correct version of Node during development. Can you please add it back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we should try to reserve the package.json published in the node module to only information needed by the consumer I think we should still leave volta out of the monorepo sub-packages package.json

Copy link
Member

Choose a reason for hiding this comment

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

If Volta adds another place to specify the development versions of node/npm then we can switch to that. But for now package.json is the only place to store pinned versions. We need to keep the volta field in CC's package.json so everyone who uses Volta will use the correct version of node.

We are following the recommendation for monorepo setups in Volta's doc:
https://docs.volta.sh/advanced/workspaces

The volta field in our package.json has no impact on our consumers, even if they use Volta. They would need to pin a version on their end.

Can you add volta back here and then I'll approve the PR? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after another discussion with Ben over teams I'm adding volta back to calcite-components

@alisonailea alisonailea requested a review from benelan October 24, 2023 16:06
@benelan
Copy link
Member

benelan commented Oct 24, 2023

That only applies to the engine listed in the calcite-components package, which doesn't currently have an engine listed at all. In the CONTRIBUTING docs it does not say you must use Volta but rather directs you to use the major version of node listed in the package.json (implying the engine key).

Following up on our Teams conversation, it looks like there was never an engines field before switching to the monorepo. I must have added it when copy/pasting from somewhere 😅

We can remove engines from the root and the newly added one in CC's package.json. Setting engines specifies the supported versions of node for using the package, and there aren't any packages being published from the root of the monorepo. I hesitate to add one to CC without doing the proper testing to determine the oldest supported version.

The supported versions for usage and the supported versions for development can be completely different, so I don't want to use the engines field for something it wasn't designed to do.

We can update the CONTRIBUTING doc to specify that the version listed under the volta field should be used for development.

@alisonailea alisonailea requested a review from a team as a code owner October 24, 2023 18:24
@alisonailea alisonailea added the low risk Issues with low risk for consideration in low risk milestones label Oct 24, 2023
Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the catch!

@alisonailea alisonailea added the skip visual snapshots Pull requests that do not need visual regression testing. label Oct 24, 2023
@alisonailea alisonailea merged commit 8eafa56 into main Oct 24, 2023
@alisonailea alisonailea deleted the astump/update-expected-node-engine branch October 24, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issues with changes that don't modify src or test files. low risk Issues with low risk for consideration in low risk milestones skip visual snapshots Pull requests that do not need visual regression testing. tooling Issues relating to build system fixes or improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants