-
Notifications
You must be signed in to change notification settings - Fork 239
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
[RRFC] Opt-in solution to support comments in package.json (among other features) #291
Comments
{
"//": "this is a comment about foo",
"foo": "this is foo's real value"
} and the actual: {
"foo": "this is a comment about foo",
"foo": "this is foo's real value"
}
|
My request addresses all concerns mentioned in that discussion. Arguments to stay with the current format:
Your argument to not allow a more permissive, opt-in, format does not fix any of the problems identified.
My solution fixes all of these problems, and has no negative side effects. Nothing with Node needs to change at all. npm would just need to ship with an additional package built in, and perform 1 extra step before certain tasks. |
None of those limitations are part of json to make it “faster for machines” - that was never part of the motivation for designing json in the first place. Separately, this isn’t purely opt-in - there are many tools in the ecosystem beyond npm itself that write to package.json, so if a project auto generated that file from an alternative manifest, all those tools would need to be able to write to that alternative manifest instead. |
I see no value in the continued discussion of the original intent of JSON's design. The fact that both of the alternatives I've mentioned have decided to market themselves as "JSON for humans" says enough. At this point it's pedantics. If third party tools want to support this opt-in feature, they can. If a user uses those tools that do not support the new format, they do not have to opt-in to the new format and continue to use the Just because some 3rd party tools would not support this out of the box, that doesn't mean we should prevent everyone from having the benefits this opt-in system gives. Most 3rd party tools would have a very easy upgrade path to support the new system, only requiring a slight change at the start and end of the code. Something like this: // Start of file
const fs = require('fs');
+const JSON5 = require('json5');
-const manifest = require('package.json');
+const newManifestFormat = fs.existsSync('./package.json5');
+const manifest = newManifestFormat ? JSON5.parse(fs.readFileSync('package.json5')) : require('package.json');
// No other code changes until the end of the file
+const stringify = newManifestFormat ? JSON5.stringify : JSON.stringify;
-const output = JSON.stringify(manifest, null, 2);
+const output = stringify(manifest, null, 2);
fs.writeFileSync('./package.json', output); These tools would remain backwards compatible, but also support the new opt-in system. The tools that do not make this simple change will continue to be used on projects that do not opt-in to the JSON5 format. |
json5 doesn’t ship with node, or in browsers - having to add an external dependency to be able to process the contents of a package manifest is a very large barrier to entry. If node, and/or browsers, were convinced to ship an alternative parser/serializer in core, then I’d say whatever they ship is a strong contender for an opt-in alternative - lacking that, i fail to see how a few niceties are worth the churn caused by allowing more than one format. |
This is a tactic called "Moving the goal posts".
Node and browsers won't add it in unless the TC39 approves. The TC39 will not approve changes to JSON because:
When installing The implementation of this feature has no unknowns and would be a small engineering effort. The documentation of this feature would require minimal effort. The only thing preventing it would be bike sheds about which technology to use (JSON5, HJSON, or something else). The negatives of this are very minor and do not outweigh the benefits. Negatives:
Benefits:
|
This topic gets a lot of FUD around it, but I support what @TheJaredWilcurt has proposed. |
I suppose completeness commends mentioning Yet Another alternative manifest format, JSON6, which claims a few advantages over JSON5. And also that pnpm already supports both JSON5 and YAML. Thank you for this request @TheJaredWilcurt |
As a developer, I often find myself struggling with the limitations of the standard JSON format when it comes to managing my project dependencies. JSON5 is an extension to the standard JSON format that includes additional features like comments and trailing commas. Despite its growing popularity among developers, NPM does not currently support JSON5 as a valid format for its package.json file. Adding support for JSON5 would make it easier for developers to manage dependencies and configurations in a more readable way, while still maintaining backward-compatibility with the standard JSON format. As a community, we must urge NPM to consider supporting the JSON5 format. |
Time to grow up npm |
What does that graph represent? |
As package.json supports more and more features, surely it makes sense to be able to document the configuration alongside it, without resorting to messy hacks? What brought me here was finding a way to document why I was using a certain override. I would have liked to be able to do: "overrides": {
"fast-json-patch": "^3.1.1" // Fix CVE https://github.com/advisories/GHSA-8gh8-hqwg-xf34
} which (even on here) is nicely styled as a comment, but instead had to settle for the somewhat clumsy: "overrides": {
"fast-json-patch__": "Fix CVE https://github.com/advisories/GHSA-8gh8-hqwg-xf34",
"fast-json-patch": "^3.1.1"
} I'd also like to note that the two comment formats suggested by @ljharb above either don't work:
or are not ideal:
|
@thscott good call on the double slash form. The other one is valid, though; duplicate keys in json aren’t an error. |
Two points here:
I think this is the wrong forum for this discussion. Luckily we are working on a new forum under the OpenJS foundation to discuss topics like this. The initial goal is to document the state of the world, but from there it is to have a place to talk about additions/changes and any related tools/docs/specs which would need to update in response. This is not the final "home" of this work, but here is where the initial research will live. In the meeting earlier this week we decided we will likely create a Collab Space for this, but there will be links and announcements in the repo linked when that happens. I would say this issue should be closed as "wont fix" and then any future conversations like this be re-directed to the collab space once it is setup. |
You've linked to what looks like a dead repo. Regardless of what the broader JS community does with Also regardless of if any other system adopted a similar approach, npm could still approve this RFC and adopt the feature now. The worst case scenario would be if the broader ecosystem adopted a different format (npm goes with JSON5, while Node goes with HJSON for example). Then npm would need to deprecate its JSON5 support and switch to HJSON. A minor annoyance while everyone converges on the same format. Though unless there is a major feature difference, whatever npm chooses will likely be the format everyone else would also adopt, if a working implementation shipped with npm first. This RFC is the 3rd most upvoted of all issues (open or closed). There is a clear desire for this functionality from the community and there are real problems this gives a practical solution to that are not solvable any other way. |
Until node offers a built-in way to serialize and unserialize JSON5 in all active release lines, it would be a disaster if npm adopted it, since tooling wouldn't be able to easily handle it. npm has an obligation here to minimize disruption, and not to pick format winners. |
It's notable that even the new shiny, bun.js, is held back from adopting a different dependency format by this constraint of NPM: oven-sh/bun#3175 Maybe PNPM is our only hope? |
@ljharb This proposal suggests automatically generating a standard package.json file, so I can't really see how it would be a disaster for npm to support it. As pointed out above, npm not moving towards supporting it is part of the reason that there isn't broader support by other tools. |
@ljharb Node does not need to do anything at all for npm to support this. This would be completely backwards compatible, because everything related to the I think the current system that forces everyone in the entire JS community to invent completely different hacky workaround solutions to deal with not being able to comment their code is the actual disaster we are living in today. Stop holding up progress. People want this. |
His point is not about this feature support when installing or publishing. He is talking about EVERYTHING else. The number of tools which read from I am not opposed to this proposal, but I do think that this discussion needs more broad discussion (not sure why that comment got downvotes). And if there was a move to support it I honestly am not sure why we wouldn't start with node core itself. Node couples to |
On that note actually, can someone explain how projects using an alternative format with |
@TheJaredWilcurt was directing addressing this point. Tools that read from package.json will still read from the generated package.json. The only stumbling block is tools that write to package.json, which are far less common. This need was also addressed by @TheJaredWilcurt (manually copy modifications over until said tools support json5 themselves). |
May I suggest Seriously, I've been asking for this change for 10 years now, so maybe it's time to give up on node.js ecosystem and go look for/build a better one. |
Edit: I simplified this section and incorporated all comments and concerns up to this point. In hopes that this can finally proceed to be implemented.
Motivation
It is now common for modern frontend/backend applications using Node to have dozens of dependencies and npm scripts, and even configurations for tools like ESLint, Jest, etc. stored in the
package.json
directly.This can make the maintenance of a project more difficult when it comes to how and were to document things related to the
package.json
file. Most commonly things around why dependencies were added, how dependencies are related, why a dependency is pinned to a specific version, and reasons for specific npm scripts. Currentlypackage.json
auto-alphabetizes dependencies, removing any intentional grouping of like-items (Example:babel-jest
,jest
,vue-jest
would logically be grouped and related to unit testing, but with a singlenpm install
they are mixed in with the rest).There is a need for a more permissive format for writing a manifest file.
Opt-in, Backwards compatible, Solution
When the
npm
executable is called and needs to access thepackage.json
(such asnpm i
,npm run
,npm publish
,npm start
,npm t
,npm audit
, etc) it will first check for apackage.json5
file. If it exists, it will process it and override the existingpackage.json
file, then proceed as usual.Benefits:
json5
have easy access to them, while those that don't need them can continue usingpackage.json
as they always have.package.json
is always kept in sync with thepackage.json5
, nothing needs to change for Node, or any other tooling. Everything still works.package.json
continues to work exactly the same.//
) and Multi-line comments (/**/
)scripts: {
)lint: 'eslint'
)Edge-cases
npm
version that handlesjson5
automaticallynpx json5 package.json5
to process the file by handDownsides (these are all very minor)
npm
would need to ship with an additional dependencies (json5
). It already ships with over 400, so this is a non-issue.package.json
directly would require repo maintainers to manually move those changes over to thepackage.json5
file. Though updating these tools to support the new format would be very easy in almost all scenarios. Example:package.json
; they may pick something other than JSON5 (TOML, HJSON, YML, whatever). npm would then need to deprecate JSON5 support as the community transitions over to the same format. I don't forsee Node ever doing this, and if they do npm's use of JSON5 would likely drive them to adopting the same format, avoiding this unlikely downside alltogether.References
pnpm
has allowed the usage ofpackage.json5
as a replacement for apackage.json
. However it does not compile apackage.json
during use, leading to members of the community to decry it as breaking backward compatibility. Thus it exists, but is not recommended by the docs.The text was updated successfully, but these errors were encountered: