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

[New Doc 📑]: specify which version of npm is required to run npm install. #427

Closed
1 task done
smoya opened this issue Dec 15, 2021 · 21 comments · Fixed by #584
Closed
1 task done

[New Doc 📑]: specify which version of npm is required to run npm install. #427

smoya opened this issue Dec 15, 2021 · 21 comments · Fixed by #584
Labels
area/docs Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. good first issue Good for newcomers released on @next-major released on @next-spec released

Comments

@smoya
Copy link
Member

smoya commented Dec 15, 2021

What Dev Docs changes are you proposing?

Current package-lock.json file sets lockfileVersion to 2. This version is only available starting from npm 7.
In the case you run npm install with a version lower than 7, it will change the version of the package-lock.json file to 1.

In order to keep consistency, I think we should definitely specify that this project requires npm version >= 7 and node version >= 14 (as per our CI https://github.com/asyncapi/parser-js/blob/master/.github/workflows/if-nodejs-pr-testing.yml#L33). In fact, better to say the best scenario is that the developer sets exactly the same versions as we do so we ensure everything works as expected.

Maybe we could add a . nodeenvrc file to set a virtual environment for npm. See https://github.com/ekalinin/nodeenv.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@derberg
Copy link
Member

derberg commented Dec 15, 2021

because there is no direct technical requirement, I don't think we should require from user that only the latest versions are supported, while in fact older work like a charm

@smoya
Copy link
Member Author

smoya commented Dec 15, 2021

Are we sure we don't want to require developers to stick to a version when in fact CI uses a particular one?
I'm talking about the combo node and npm. If you don't, thinks like what I mentioned will happen:

In the case you run npm install with a version lower than 7, it will change the version of the package-lock.json file to 1.

Should then package-lock.json change depending on the setup of latest person who committed code after running npm install?

Regarding node version, how do you ensure compatibility? That's why CI it's in place but wouldn't be easier for developers to have a setup closer to the same used when publishing packages?

@derberg
Copy link
Member

derberg commented Dec 15, 2021

yeah, so I just think it's better to improve testing and test on older versions to make it easier for folks that are in some enterprise are stuck with some old version.

according to https://nodejs.org/en/about/releases/ Node 12 is still there in maintenance until April 2022.

but yeah, I'm talking here specifically about node, about npm do not have much of an opinion because I did not have time to really read about lockfileVersion1 vs lockfileVersion2

@smoya
Copy link
Member Author

smoya commented Dec 15, 2021

The point here is that I ran this simple npm install command and package-lock.json got modified without adding or removing any dependency. I think this should not happen at all. We can't really avoid that but at least guide developers and set expectations on their setup so they are aware of this.

@derberg
Copy link
Member

derberg commented Dec 15, 2021

yeah, this part I agree, it is confusing, just don't think solution should be about restricting to certain npm/node. It is not only about this repo, also other node projects really.

Interesting what others think

@smoya
Copy link
Member Author

smoya commented Dec 16, 2021

Pinging the rest of code owners in a first attempt on getting feedback about this issue @fmvilas @magicmatatjahu @jonaslagoni

@magicmatatjahu
Copy link
Member

@smoya you have right. Someone changes a single line and suddenly has several thousand changes in package-lock.json. We should force using npm >7 or <7 to avoid such situations.

@smoya
Copy link
Member Author

smoya commented Dec 16, 2021

An very recent example of this issue, that shows that not only applies to this repo but to any other that uses npm:

asyncapi/studio#201 (comment)

There was no new addition or removal of packages, but still a big diff in package-lock.json is there in the PR because of npm version missmatch.

@fmvilas
Copy link
Member

fmvilas commented Dec 16, 2021

I'd not force to a specific version of npm but would recommend it for contributors. If it works with an older version of npm we should not enforce it IMHO. Only contributors would care about package-lock.json being changed, right? Users would not see any issues as the changes would happen under node_modules which is usually ignored.

@smoya
Copy link
Member Author

smoya commented Dec 16, 2021

Only contributors would care about package-lock.json being changed, right?

Exactly, the issue only affects developers. That's why I mention that it is an issue when running npm install on this package, something you won't do if you are not a developer of this package.

Besides clarifying this in the documentation, Isn't this .nodeenvrc stuff a common thing ? (I am completely new to this world 😅 )

@derberg
Copy link
Member

derberg commented Dec 20, 2021

I never used . nodeenvrc. If you really need to be strict on versions, you do something like https://github.com/asyncapi/generator/blob/master/package.json#L10-L13 and then update readme https://github.com/asyncapi/generator#requirements

I think you brought important topic of package-lock.json. I don't think we should force but I think we should definitely have it documented and would be nice to alight that in all libraries we want to follow lockfileVersion: 2

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Apr 20, 2022
@smoya
Copy link
Member Author

smoya commented Apr 20, 2022

@magicmatatjahu would you mind removing stale label, and maybe add a good-first-issue instead?

@derberg
Copy link
Member

derberg commented Aug 18, 2022

@smoya just to clarify, goal of this doc is just to write in https://github.com/asyncapi/parser-js#develop Develop section that latest Node and NPM is required?

@smoya
Copy link
Member Author

smoya commented Aug 19, 2022

@smoya just to clarify, goal of this doc is just to write in https://github.com/asyncapi/parser-js#develop Develop section that latest Node and NPM is required?

After reading all the comments on this issue again, I think that's exactly what we need.

@derberg
Copy link
Member

derberg commented Aug 22, 2022

/gfi docs

@asyncapi-bot asyncapi-bot added the area/docs Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. label Aug 22, 2022
@derberg
Copy link
Member

derberg commented Sep 14, 2022

I think we need something more. Docs is great, but we need a GH Action that will validate it on a PR level -> https://github.com/asyncapi/parser-js/blob/master/package-lock.json#L4 -> a result of the release from yesterday 🤷🏼

@derberg
Copy link
Member

derberg commented Sep 15, 2022

fyi asyncapi/.github#177

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 1.17.0-next-spec.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 1.17.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 2.0.0-next-major.18 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. good first issue Good for newcomers released on @next-major released on @next-spec released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants