parser-js

parser-js copied to clipboard

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

• [X] I agree to follow this project's Code of Conduct

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

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?

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

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.

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

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

@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.

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

https://github.com/asyncapi/studio/pull/201#issuecomment-995707003

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.

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.

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 😅 )

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

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

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 :heart:

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

@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 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.

/gfi docs

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 🤷🏼

fyi https://github.com/asyncapi/.github/issues/177

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

The release is available on:

• npm package (@next-spec dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

:tada: This issue has been resolved in version 1.17.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

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

The release is available on:

• npm package (@next-major dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: