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

[FEATURE] Allow jsonc or JSON5 for package.json for commenting #793

Closed
admosity opened this issue Feb 10, 2020 · 23 comments
Closed

[FEATURE] Allow jsonc or JSON5 for package.json for commenting #793

admosity opened this issue Feb 10, 2020 · 23 comments
Labels
Enhancement new feature or improvement

Comments

@admosity
Copy link

What / Why

For better or for worse the content within package.json has been growing with greater and greater complexity. In the nascent days of npm, the file was simply just some metadata on the dependencies that a project/package depended on. Projects grew in complexity and now testing, frameworks, other libraries are all dependencies that would appear sitting together. There was some separation with dependencies and devDependencies, but that is a rough categorization. We have tools now contributing and taking residence within the package.json with greater complexity... monorepo configurations, lint configurations, commit configurations, git hooks, nodemon, jest, the list goes on...

Comments offer a way to explain all the complexity.

I still think that library maintainers should still be using pure JSON for backwards compatibility, but at least offering this moving forward will allow project creators (the vast majority of users) the ability to document their package.json. Granted, json allows for duplicate keys to serve as comments, but this feels too much like a hack.

How

Current Behavior

A redacted and contrived example:

{
  "name": "my-project",
  "version": "0.0.1",
  "license": "MIT",
  "scripts": {
    "build": "a bunch of scripts",
    "ci": "a bunch of scripts",
    "ci:local": "a bunch of scripts",
    "docsite": "a bunch of scripts",
    "docsite:combiner": "a bunch of scripts",
    "docsite:sassdoc": "a bunch of scripts",
    "docsite:tsdoc": "a bunch of scripts",
    "e2e": "a bunch of scripts",
    "html-sketchapp-install": "a bunch of scripts",
    "html-sketchapp": "a bunch of scripts",
    "lint": "a bunch of scripts",
    "sassdoc": "a bunch of scripts",
    "sassdoc:comp": "a bunch of scripts",
    "sassdoc:core": "a bunch of scripts",
    "start": "a bunch of scripts",
    "start:hmr": "a bunch of scripts",
    "start:qa": "a bunch of scripts",
    "start:dev": "a bunch of scripts",
    "generate-examples": "a bunch of scripts",
    "rebuild-markdown": "a bunch of scripts",
    "watch-examples": "a bunch of scripts",
    "test:cov": "a bunch of scripts",

    "test": "jest",
    "test:watch": "jest --watch",
    "test:cc": "jest --coverage"
  },
  "config": {
    "commitizen": {
      "path": "./node_modules/cz-conventional-changelog"
    }
  },
  "dependencies": {
    "@angular-devkit/build-angular": "~0.900.1",
    "@angular-devkit/build-ng-packagr": "~0.900.1",
    "@angular-devkit/core": "~9.0.0",
    "@angular-devkit/schematics": "~9.0.0",
    "@angular/animations": "~9.0.0",
    "@angular/cdk": "~9.0.0",
    "@angular/cli": "~9.0.1",
    "@angular/common": "~9.0.0",
    "@angular/compiler": "~9.0.0",
    "@angular/compiler-cli": "~9.0.0",
    "@angular/core": "~9.0.0",
    "@angular/forms": "~9.0.0",
    "@angular/language-service": "~9.0.0",
    "@angular/platform-browser": "~9.0.0",
    "@angular/platform-browser-dynamic": "~9.0.0",
    "@angular/router": "~9.0.0",
    "@angular/service-worker": "~9.0.0",
    "@angularclass/hmr": "2.1.3",
    "@ngrx/effects": "~8.6.0",
    "@ngrx/entity": "~8.6.0",
    "@ngrx/router-store": "~8.6.0",
    "@ngrx/schematics": "~8.6.0",
    "@ngrx/store": "~8.6.0",
    "@ngrx/store-devtools": "~8.6.0",
    "@types/jasmine": "~3.5.0",
    "@types/jasminewd2": "~2.0.3",
    "@types/lodash-es": "^4.17.3",
    "@types/node": "^12.11.1",
    "classlist.js": "1.1.20150312",
    "codelyzer": "^5.1.2",
    "console-polyfill": "0.3.0",
    "core-js": "^2.5.4",
    "cz-conventional-changelog": "1.2.0",
    "date-fns": "1.30.1",
    "highcharts": "^7.2.1",
    "highcharts-angular": "^2.4.0",
    "html2canvas": "^1.0.0-rc.5",
    "jasmine-core": "~3.5.0",
    "jasmine-spec-reporter": "~4.2.1",
    "karma": "~4.3.0",
    "karma-chrome-launcher": "~3.1.0",
    "karma-coverage-istanbul-reporter": "~2.1.0",
    "karma-jasmine": "~2.0.1",
    "karma-jasmine-html-reporter": "^1.4.2",
    "lodash-es": "^4.17.11",
    "mime": "~2.4.2",
    "ngrx-store-freeze": "0.2.4",
    "ngx-monaco-editor": "~8.0.0",
    "ngx-quill": "^7.3.12",
    "pdfmake": "^0.1.64",
    "prettier": "1.19.1",
    "protractor": "~5.4.3",
    "quill": "^1.3.7",
    "rxjs": "~6.5.4",
    "rxjs-compat": "^6.0.0",
    "standard-changelog": "1.0.19",
    "svgxuse": "1.2.6",
    "ts-node": "~8.3.0",
    "tsickle": "^0.35.0",
    "tslib": "^1.10.0",
    "tslint": "~5.18.0",
    "typescript": "~3.7.5",
    "web-animations-js": "~2.3.1",
    "zone.js": "~0.10.2"
  },
  "devDependencies": {
    "@types/jest": "^24.0.6",
    "jest": "^24.1.0",
    "jest-preset-angular": "^6.0.2",
    "ts-node": "~7.0.1",
    "typescript": "3.2.4"
  },
  "jest": {
    "preset": "jest-preset-angular",
    "setupTestFrameworkScriptFile": "<rootDir>/setupJest.ts"
  },
  "nodemonConfig": {
    "ignore": [
      "**/example-module.ts"
    ],
    "watch": [
      "./a/bunch/of/stuff"
    ],
    "ext": "js ts md html"
  },
  "workspaces": {
    "packages": [
      "packages/*"
    ],
    "nohoist": [
      "**"
    ]
  }
}

Expected Behavior

An example of a package.json with comments.

{
  "name": "my-project",
  "version": "0.0.1",
  "license": "MIT",
  "scripts": {
    "build": "a bunch of scripts",
    "ci": "a bunch of scripts",
    "ci:local": "a bunch of scripts",
    // for building our documentation site
    "docsite": "a bunch of scripts",
    "docsite:combiner": "a bunch of scripts",
    "docsite:sassdoc": "a bunch of scripts",
    "docsite:tsdoc": "a bunch of scripts",
    "e2e": "a bunch of scripts",
    "lint": "a bunch of scripts",
    "sassdoc": "a bunch of scripts",
    "sassdoc:comp": "a bunch of scripts",
    "sassdoc:core": "a bunch of scripts",
    "start": "a bunch of scripts",
    "start:hmr": "a bunch of scripts",
    "start:qa": "a bunch of scripts",
    "start:dev": "a bunch of scripts",
    "generate-examples": "a bunch of scripts",
    "rebuild-markdown": "a bunch of scripts",
    "watch-examples": "a bunch of scripts",
    "test:cov": "a bunch of scripts",

    "test": "jest",
    "test:watch": "jest --watch",
    "test:cc": "jest --coverage"
  },
  "config": {
    "commitizen": {
      "path": "./node_modules/cz-conventional-changelog"
    }
  },
  "dependencies": {
    // ANGULAR
    "@angular-devkit/build-angular": "~0.900.1",
    "@angular-devkit/build-ng-packagr": "~0.900.1",
    "@angular-devkit/core": "~9.0.0",
    "@angular-devkit/schematics": "~9.0.0",
    "@angular/animations": "~9.0.0",
    "@angular/cdk": "~9.0.0",
    "@angular/cli": "~9.0.1",
    "@angular/common": "~9.0.0",
    "@angular/compiler": "~9.0.0",
    "@angular/compiler-cli": "~9.0.0",
    "@angular/core": "~9.0.0",
    "@angular/forms": "~9.0.0",
    "@angular/language-service": "~9.0.0",
    "@angular/platform-browser": "~9.0.0",
    "@angular/platform-browser-dynamic": "~9.0.0",
    "@angular/router": "~9.0.0",
    "@angular/service-worker": "~9.0.0",
    "@angularclass/hmr": "2.1.3",

    "zone.js": "~0.10.2",
    "rxjs": "~6.5.4",
    "rxjs-compat": "^6.0.0",


    // polyfills for angular 
    "console-polyfill": "0.3.0",
    "core-js": "^2.5.4",
    "classlist.js": "1.1.20150312",
    "svgxuse": "1.2.6",
    "web-animations-js": "~2.3.1",

    // lint management

    "codelyzer": "^5.1.2",
    "prettier": "1.19.1",
    "tslint": "~5.18.0",

    // changelog creation
    "cz-conventional-changelog": "1.2.0",
    "standard-changelog": "1.0.19",

    // ngrx state management 
    "@ngrx/effects": "~8.6.0",
    "@ngrx/entity": "~8.6.0",
    "@ngrx/router-store": "~8.6.0",
    "@ngrx/schematics": "~8.6.0",
    "@ngrx/store": "~8.6.0",
    "@ngrx/store-devtools": "~8.6.0",
    "ngrx-store-freeze": "0.2.4",


    // library deps

    // Graphs support
    "highcharts": "^7.2.1",
    "highcharts-angular": "^2.4.0",

    // for creating pdfs
    "html2canvas": "^1.0.0-rc.5",
    "pdfmake": "^0.1.64",

    // rich text support
    "ngx-quill": "^7.3.12",
    "quill": "^1.3.7",




    // testing
    "jasmine-core": "~3.5.0",
    "jasmine-spec-reporter": "~4.2.1",
    "karma": "~4.3.0",
    "karma-chrome-launcher": "~3.1.0",
    "karma-coverage-istanbul-reporter": "~2.1.0",
    "karma-jasmine": "~2.0.1",
    "karma-jasmine-html-reporter": "^1.4.2",

    // e2e testing
    "protractor": "~5.4.3",

    // typscript and compilation
    "ts-node": "~8.3.0",
    "tsickle": "^0.35.0",
    "tslib": "^1.10.0",
    "typescript": "~3.7.5",

    // misc deps
    "date-fns": "1.30.1",
    "lodash-es": "^4.17.11",
    "mime": "~2.4.2"
  },
  "devDependencies": {
    "@types/jasmine": "~3.5.0",
    "@types/jasminewd2": "~2.0.3",
    "@types/lodash-es": "^4.17.3",
    "@types/node": "^12.11.1",
    "@types/jest": "^24.0.6",
    "jest": "^24.1.0",
    "jest-preset-angular": "^6.0.2",
    "ts-node": "~7.0.1",
    "typescript": "3.2.4"
  },
  // complicated jest configuration here
  "jest": {
    "preset": "jest-preset-angular",
    "setupTestFrameworkScriptFile": "<rootDir>/setupJest.ts"
  },
  // use nodemon to trigger stuff
  "nodemonConfig": {
    "ignore": [
      "**/example-module.ts"
    ],
    "watch": [
      "./a/bunch/of/stuff"
    ],
    "ext": "js ts md html"
  },
  // extra libraries we are creating internally
  "workspaces": {
    "packages": [
      "packages/*"
    ],
    "nohoist": [
      "**"
    ]
  }
}

References

https://github.com/microsoft/node-jsonc-parser

Typescript uses comments in their tsconfig.json files.

@burtek
Copy link

burtek commented Feb 17, 2020

@admosity unfortunatelly, this would be backwards incompatible :/ Also, it would require all other software that uses package.json, as well as many npm packages, to understand the new syntax...

@DanielRuf
Copy link

See #677

@admosity admosity changed the title [FEATURE] Allow jsonc or JSON5 for package.json [FEATURE] Allow jsonc or JSON5 for package.json for commenting Feb 25, 2020
@dan-ryan
Copy link

dan-ryan commented Mar 29, 2020

this would be backwards incompatible

You could have two files to keep it backwards compatible:
package.json
package.json5

The user would just edit package.json5 to add the comments. Npm could create (or override) package.json and just ignore the comments from package.json5.

@mindplay-dk
Copy link

Any new feature is by definition backwards incompatible - isn't that normal and in the nature of progressing in the evolution of any piece of software?

Besides being very easily fixed in other projects that load package.json, by simply adding the package and replacing JSON with JSON5 - the APIs are identical.

Babel added JSON5 support a while ago and the community just moves forward and fixes these tiny issues, which takes no time at all - and the improvements of JSON5 just sort of spreads organically to other packages, documentation improvements spreads to projects, and so on.

That seems like a good thing.

IMO, don't add more complexity/confusion just to provide an excuse for not moving forward.

@darcyclarke darcyclarke added the Enhancement new feature or improvement label Oct 30, 2020
@jonlepage
Copy link

with vscode we can set the parser to jsonc "json with comment"
It just stupid we cant doc the package.json ! it the most important file in the root of project for dev and understand a project!
image

@snebjorn
Copy link

snebjorn commented Mar 11, 2021

I find comments invaluable! Currently we have to just remember or keep a separate document for stuff like this.

{
  "name": "comments-pls",
  "version": "0.0.0",
  "private": true,
  "dependencies": {
      "fooPkg": "^6.6.6" // don't update until <link to issue> have been fixed
  },
  "devDependencies": {
    "husky": "^4.3.8" // WARN do not update to 5 the license have changed
  }
}

And no one looks at this sparate document until the house is burning. So in reality we just have to remember. And remember to tell new team members that they have to remember. It's not ideal.

JSON5 also supports trailing commas. Yet another quality of dev life feature that is very welcome.

@ljharb
Copy link
Contributor

ljharb commented Mar 11, 2021

JSON already supports comments:

{
  "foo": "foo is an important field!",
  "foo": "the real value of foo"
}

I’d love to see npm ensure these aren’t dropped whenever it changed package.json.

@snebjorn
Copy link

snebjorn commented Mar 11, 2021

To me that looks like unintended usage to hack a workaround rather then support.

VS Code doesn't like it either
image

@ljharb
Copy link
Contributor

ljharb commented Mar 11, 2021

It's quite intended, since JSON explicitly allows duplicate keys.

vscode being wrong doesn't make it a hack :-)

@AlexWayfer
Copy link

It's quite intended, since JSON explicitly allows duplicate keys.

vscode being wrong doesn't make it a hack :-)

Can you please proof your words? I didn't find info about in at json.org, and the question leads me here: https://stackoverflow.com/a/21833017/2630849

It does not make any mention of duplicate keys being invalid or valid, so according to the specification I would safely assume that means they are allowed.

@ljharb
Copy link
Contributor

ljharb commented Mar 12, 2021

@mindplay-dk
Copy link

It's quite intended, since JSON explicitly allows duplicate keys.

vscode being wrong doesn't make it a hack :-)

I find both of these statements to be somewhat inaccurate.

Quoting section 3 of the ECMA-404 spec:

The JSON syntax does not impose any restrictions on the strings used as names, does not require that name strings be unique, and does not assign any significance to the ordering of name/value pairs.

"Does not require" does not imply "explicitly allows" - and, in fact, it goes on to clarify:

These are all semantic considerations that may be defined by JSON processors or in specifications defining specific uses of JSON for data interchange.

In other words, the behavior is explicitly undefined.

VSCode can't be "wrong" on this point, since the behavior is explicitly undefined - it's really up to anyone what they want to do.

Highlighting this as an error seems completely appropriate - since, per the specification, you can't know how an implementation will handle this, the only safe option is to treat this like an error.

There's simply no right way to comment an JSON file.

Only hacks.

Which is what we're stuck with at the moment.

@AlexWayfer
Copy link

See https://stackoverflow.com/questions/21832701/does-json-syntax-allow-duplicate-keys-in-an-object/21833017#comment33046713_21833017 and section 6 in the standard, which says “ does not require that name strings be unique” (https://www.ecma-international.org/wp-content/uploads/ECMA-404_2nd_edition_december_2017.pdf).

Thank you.

It's quite intended, since JSON explicitly allows duplicate keys.
vscode being wrong doesn't make it a hack :-)

I find both of these statements to be somewhat inaccurate.

Quoting section 3 of the ECMA-404 spec:

The JSON syntax does not impose any restrictions on the strings used as names, does not require that name strings be unique, and does not assign any significance to the ordering of name/value pairs.

"Does not require" does not imply "explicitly allows" - and, in fact, it goes on to clarify:

These are all semantic considerations that may be defined by JSON processors or in specifications defining specific uses of JSON for data interchange.

In other words, the behavior is explicitly undefined.

VSCode can't be "wrong" on this point, since the behavior is explicitly undefined - it's really up to anyone what they want to do.

Highlighting this as an error seems completely appropriate - since, per the specification, you can't know how an implementation will handle this, the only safe option is to treat this like an error.

There's simply no right way to comment an JSON file.

Only hacks.

Which is what we're stuck with at the moment.

I agree. And, for example, it means, that some JSON parsers can count the first duplicate key, others can the last one, there is no standard, guarantee, hereby (hacky) support of comments, returning to the subject.

@jcrben
Copy link
Contributor

jcrben commented Apr 4, 2021

There is https://github.com/wiredmax/npm-json5 which works for npm 3. With a bit of love, I suspect later versions could be supported also. That's a less controversial way to transition

@RobertLowe
Copy link

Simply allowing for .jsonc alternative extension for local projects would fill 100% of my use cases. As for published and future packages they can remain regular .json.

In the meantime, as part of your build/install process you could use https://github.com/sindresorhus/strip-json-comments-cli to generate a json file.

@mindplay-dk
Copy link

Simply allowing for .jsonc alternative extension for local projects would fill 100% of my use cases. As for published and future packages they can remain regular .json.

Probably not that simple, as many other tools (and websites etc) read and parse the package.json file.

There is https://github.com/wiredmax/npm-json5 which works for npm 3.

Same problem here, as other tools may not be able to parse the package.json file.

If npm officially supported JSON5, that would be a different situation - everyone would expect to have to upgrade their packages and web-services etc.

Probably the best bet in either case though (whether officially supported or via 3rd party add-in) is to use an alternative file-name, e.g. package.jsonc from which npm would generate the package.json file anytime you run the tool. Even that could get confusing for contributors, who would naturally expect to just edit the package.json file, but I don't see how else we could avoid the disruption of the ecosystem around npm?

@jonlepage
Copy link

Probably not that simple, as many other tools (and websites etc) read and parse the package.json file.

Where would all the big softwares be today without all these BreakChanges updates.
Sure is always frustrating that my Adobe scripts broke from version to new version.
But that's the definition of progress, we have to change things for the better thing.
In this case it will maybe be a nice Breakchange, and people just have to update and follow progress.

Progressive ! Do not stagnate in mediocrity for fear of angering the conservatives.

@RunAge
Copy link

RunAge commented Apr 6, 2021

I think npm could just on publish parse JSON5 and create package.json if package.json5 exists. Also we are developers and most of us use comments, tailing comma(better git compatibility) but if someone use or package/module don't care if use use JSON or JSON5 in this case. Another feature which comes with JSON5 is long scripts/commands can be split to new lines.

TLDR:
JSON5 give developers more control of package.json
JSON5 can be omitted on publish like tests etc. and only put parsed package.json

@mindplay-dk
Copy link

I think npm could just on publish parse JSON5 and create package.json if package.json5 exists.

I expect a lot of sites parse package.json directly from repositories. For example, how else would Github know when a repository is a package, being able to display a list of projects that use the package, and so on?

I suppose these tools would need to upgrade - just that this could be disruptive to the ecosystem during a transition period. If people start renaming the package file, a lot of tools will lose track of the fact that these repositories were packages.

@mindplay-dk
Copy link

To be clear, I'm not saying that means we shouldn't do it! I just mean to create awareness of the kind of problems we should prepare for.

@RobertLowe
Copy link

I expect a lot of sites parse package.json directly from repositories. For example, how else would Github know when a repository is a package, being able to display a list of projects that use the package, and so on?

I think they could adapt, it might be a breaking change but thats what semver is for.

@mindplay-dk
Copy link

mindplay-dk commented Apr 28, 2021

I think they could adapt, it might be a breaking change but thats what semver is for.

Again, not to say we shouldn't do it, but semantic versioning doesn't help here, at all.

I'm talking about third-party services that read and parse package.json files independently of npm or any package.

@darcyclarke
Copy link
Contributor

npm v6 is no longer in active development; We will continue to push security releases to v6 at our team's discretion as-per our Support Policy.

If your bug is preproducible on v7, please re-file this issue using our new issue template.

If your issue was a feature request, please consider opening a new RRFC or RFC. If your issue was a question or other idea that was not CLI-specific, consider opening a discussion on our feedback repo

Closing: This is an automated message.

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

No branches or pull requests