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: require node >=10 #302

Merged
merged 3 commits into from
Apr 2, 2020
Merged

chore: require node >=10 #302

merged 3 commits into from
Apr 2, 2020

Conversation

varl
Copy link
Contributor

@varl varl commented Mar 25, 2020

Supersedes #299

We want to bump our required Node version to >=10 since some of the sub-packages (cli-style, cli-app-scripts, cli-utils-cypress) already use dependencies requiring those Node versions.

Dependabot also does not look closely at the engines field to determine compatibility so we need to make sure we are up-to-date ourselves.

BREAKING CHANGE: Require Node version 10 or later.

@amcgee
Copy link
Member

amcgee commented Mar 25, 2020

Should we move to bundling Node and producing standalone executables soon?

@varl
Copy link
Contributor Author

varl commented Mar 26, 2020

Yeah, but there's still the question about how to deal with the external sub dependencies like cli-utils-cypress, cli-style, et. al.

@amcgee
Copy link
Member

amcgee commented Mar 26, 2020

Hmm but I think those would be bundled as well?

@varl
Copy link
Contributor Author

varl commented Mar 27, 2020

Hmm but I think those would be bundled as well?

Yes, ultimately. What I mean is that all of our external sub-packages can define different versions of Node, so we would still have to keep track of when an external sub-package breaks compatibility, we need to bump the Node version in CLI as well.

@amcgee
Copy link
Member

amcgee commented Apr 1, 2020

I think this is fine to merge, will do a quick test. @varl just curious, why is the fs-extra upgrade tagged as a breaking change though?

@varl
Copy link
Contributor Author

varl commented Apr 1, 2020

That's a fair question. Bumping fs-extra from 8 to 9 is why I started looking into bumping node to >=10, as they list that as a breaking change in their changelog as well: https://github.com/jprichardson/node-fs-extra/blob/master/CHANGELOG.md#900--2020-03-19

The question is how should we handle when deps cause us to release breaking changes? Do we mark them? Do we just silently update?

Do we consider CLI it's own thing and not bump the major in CLI when e.g. cli-style releases a breaking change?

@amcgee
Copy link
Member

amcgee commented Apr 1, 2020

For this change I think the Node engine update is the breaking change, the fs-extra change doesn't change anything for the user of the CLI. I think that's the general philosophy for me - if a user of cli might have written a script that would break because of a dependency change in cli, then that's a breaking change in cli (so if we clobber an existing API in cli-style, for instance, that's a breaking cli change. Adding a feature to cli-style is not). I don't know that we need to be SUPER strict about this for tiny breaking changes in dependencies, but generally should follow those guidelines

Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

Looks good, tested that the install breaks when using Node v8. The CLI will still run with non-compliant Node versions if it's already been installed, though - we should look into adding something like check-engine (that one's a bit older) to cli-helpers-engine so that we always check the Node and Yarn versions match what we need.

@amcgee
Copy link
Member

amcgee commented Apr 1, 2020

BREAKING CHANGE: Require Node version 10 or later.
BREAKING CHANGE: Update fs-extra to 9.x.

To conclude I think this is enough as the Breaking Change in the changelog (since it's the underlying reason for the fs-extra upgrade as well)

@amcgee
Copy link
Member

amcgee commented Apr 1, 2020

Should we update the engine in cli-style, cli-app-scripts, etc. too?

BREAKING CHANGE: Require Node version 10 or later.
@varl
Copy link
Contributor Author

varl commented Apr 1, 2020

Should we update the engine in cli-style, cli-app-scripts, etc. too?

Yes.

@varl varl merged commit daad89e into master Apr 2, 2020
@varl varl deleted the update-node-versions branch April 2, 2020 12:47
@dhis2-bot
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

3 participants