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

JSON5 support? #83

Closed
mathieumg opened this issue Jun 9, 2015 · 16 comments
Closed

JSON5 support? #83

mathieumg opened this issue Jun 9, 2015 · 16 comments

Comments

@mathieumg
Copy link

Is there JSON5 support planned? If not, would you be open to a PR that adds it? We use https:/rlidwka/yapm which supports package.json5 files. The change would be simple enough, you'd need to be using https:/rlidwka/jju directly (you're somewhat already using it if the parse fails, see below) to manipulate the package.json5 file, you'd then only need to lookup for a package.json5 first (or provide a switch to specify the filename, or something like that).

Regarding jju, you're currently using https:/tjunnone/npm-check-updates/blob/master/lib/npm-check-updates.js#L11 which uses https:/npm/read-package-json/blob/master/read-json.js#L12 which in turn uses https:/smikes/json-parse-helpfulerror/blob/master/index.js#L3 when the parse fails. jju is able to manipulate and save to a JSON5 file even when there are comments or other JSON5 specifics in it, see the live demo: https://rlidwka.github.io/jju/editor.html

@raineorshine
Copy link
Owner

This sounds like a great idea! If the json reading can be abstracted away
from the logic, it shouldn’t be a problem. I’d be happy to accept a pull
request and provide advisement on any questions.

I have been refactoring a lot lately, in preparation for v2, but it
shouldn’t be changing too much now.

On Tue, Jun 9, 2015 at 11:08 AM, Mathieu M-Gosselin <
[email protected]> wrote:

Is there JSON5 support planned? If not, would you be open to a PR that
adds it? We use https:/rlidwka/yapm which supports
package.json5 files. The change would be simple enough, you'd need to be
using https:/rlidwka/jju directly (you're somewhat already
using it if the parse fails, see below) to manipulate the package.json5
file, you'd then only need to lookup for a package.json5 first (or provide
a switch to specify the filename, or something like that).

Regarding jju, you're currently using
https:/tjunnone/npm-check-updates/blob/master/lib/npm-check-updates.js#L11
which uses
https:/npm/read-package-json/blob/master/read-json.js#L12
which in turn uses
https:/smikes/json-parse-helpfulerror/blob/master/index.js#L3
when the parse fails. jju is able to manipulate and save to a JSON5 file
even when there are comments or other JSON5 specifics in it, see the live
demo: https://rlidwka.github.io/jju/editor.html


Reply to this email directly or view it on GitHub
#83.

@raineorshine
Copy link
Owner

read-package-json removed

@mathieumg
Copy link
Author

Oh, I missed that it wasn't used. Yeah, the JSON reading would be abstracted, I'll try to tackle this soon when I get a couple spare seconds! 😁

@mathieumg
Copy link
Author

I'm working on integrating this, but I noticed that master is only reporting a few available updates compared to alpha 9, do you want me to open a separate issue for that?

2.0.0 - Alpha 9:
"async" can be updated from ^1.0.0 to ^1.2.1 (Installed: 1.0.0, Latest: 1.2.1)
"babel-core" can be updated from ^5.4.7 to ^5.5.8 (Installed: 5.4.7, Latest: 5.5.8)
"body-parser" can be updated from ^1.12.4 to ^1.13.1 (Installed: 1.12.4, Latest: 1.13.1)
"compression" can be updated from 1.4.4 to 1.5.0 (Installed: 1.4.4, Latest: 1.5.0)
"csurf" can be updated from ^1.8.2 to ^1.8.3 (Installed: 1.8.2, Latest: 1.8.3)
"express-session" can be updated from ^1.11.2 to ^1.11.3 (Installed: 1.11.2, Latest: 1.11.3)
"lodash" can be updated from ^3.9.2 to ^3.9.3 (Installed: 3.9.2, Latest: 3.9.3)
"morgan" can be updated from ^1.5.3 to ^1.6.0 (Installed: 1.5.3, Latest: 1.6.0)
"serve-favicon" can be updated from ^2.2.1 to ^2.3.0 (Installed: 2.2.1, Latest: 2.3.0)
"validator" can be updated from ^3.40.0 to ^3.40.1 (Installed: 3.40.0, Latest: 3.40.1)
"yargs" can be updated from ^3.10.0 to ^3.11.0 (Installed: 3.10.0, Latest: 3.11.0)
"babel-loader" can be updated from ^5.1.3 to ^5.1.4 (Installed: 5.1.3, Latest: 5.1.4)
"bootstrap" can be updated from ^3.3.4 to ^3.3.5 (Installed: 3.3.4, Latest: 3.3.5)
"css-loader" can be updated from ^0.14.4 to ^0.15.1 (Installed: 0.14.5, Latest: 0.15.1)
"extract-text-webpack-plugin" can be updated from ^0.8.1 to ^0.8.2 (Installed: 0.8.1, Latest: 0.8.2)
"react-bootstrap" can be updated from ^0.22.6 to ^0.23.4 (Installed: 0.22.6, Latest: 0.23.4)
"react-a11y" can be updated from ^0.1.1 to ^0.2.6 (Installed: 0.1.1, Latest: 0.2.6)
"react-fa" can be updated from ^3.2.0 to ^3.3.1 (Installed: 3.2.0, Latest: 3.3.1)
"react-typeahead" can be updated from ^1.0.5 to ^1.0.8 (Installed: 1.0.5, Latest: 1.0.8)
"react-widgets" can be updated from ^2.7.0 to ^2.7.1 (Installed: 2.7.0, Latest: 2.7.1)
"spin.js" can be updated from ^2.1.0 to ^2.3.1 (Installed: 2.1.0, Latest: 2.3.1)
"webpack" can be updated from ~1.9.10 to ~1.9.11 (Installed: 1.9.10, Latest: 1.9.11)
master - 3cf3d8a:
"compression" can be updated from 1.4.4 to 1.5.0 (Installed: 1.4.4, Latest: 1.5.0)
"css-loader" can be updated from ^0.14.4 to ^0.15.1 (Installed: 0.14.5, Latest: 0.15.1)
"react-bootstrap" can be updated from ^0.22.6 to ^0.23.4 (Installed: 0.22.6, Latest: 0.23.4)
"react-a11y" can be updated from ^0.1.1 to ^0.2.6 (Installed: 0.1.1, Latest: 0.2.6)
package.json :
{
    "name": "example",
    "version": "0.0.1",
    "devDependencies": {
        "accounting": "^0.4.1",
        "autoprefixer-loader": "^2.0.0",
        "babel-loader": "^5.1.3",
        "bootstrap": "^3.3.4",
        "bower-webpack-plugin": "^0.1.8",
        "codemirror": "^5.3.0",
        "css-loader": "^0.14.4",
        "exports-loader": "^0.6.2",
        "expose-loader": "^0.7.0",
        "extract-text-webpack-plugin": "^0.8.1",
        "file-loader": "^0.8.4",
        "i18next-client": "^1.9.0",
        "i18next-static-analysis": "^0.0.2",
        "jquery": "^2.1.4",
        "jquery-form": "^3.50.0",
        "json-loader": "^0.5.2",
        "json5-loader": "^0.6.0",
        "less": "^2.5.1",
        "less-loader": "^2.2.0",
        "moment": "^2.10.3",
        "react-bootstrap": "^0.22.6",
        "react-a11y": "^0.1.1",
        "react-fa": "^3.2.0",
        "react-hot-loader": "^1.2.7",
        "react-typeahead": "^1.0.5",
        "react-widgets": "^2.7.0",
        "react-widgets-moment-localizer": "^1.0.1",
        "spin.js": "^2.1.0",
        "style-loader": "^0.12.3",
        "summernote": "0.6.7",
        "superagent": "^1.2.0",
        "url-loader": "^0.5.6",
        "webpack": "~1.9.10",
        "webpack-dev-server": "^1.9.0",
        "world-countries": "^1.7.3",
        "world-currencies": "0.0.5"
    },
    "dependencies": {
        "async": "^1.0.0",
        "babel-core": "^5.4.7",
        "body-parser": "^1.12.4",
        "compression": "1.4.4",
        "connect-flash": "0.1.x",
        "connect-html-minifier": "0.1.0",
        "connect-redis": "2.x",
        "cookie-parser": "^1.3.5",
        "csurf": "^1.8.2",
        "ewait": "~0.2.1",
        "express": "4.12.x",
        "express-session": "^1.11.2",
        "forwarded-for": "^1.0.0",
        "i18next": "^1.9.0",
        "json5": "^0.4.0",
        "lodash": "^3.9.2",
        "mailchimp-api": "^2.0.7",
        "maxmind": "^0.5.5",
        "morgan": "^1.5.3",
        "multer": "^0.1.8",
        "node-uuid": "^1.4.3",
        "numeral": "^1.5.3",
        "passport": "^0.2.2",
        "passport-facebook": "^2.0.0",
        "passport-google-oauth": "^0.2.0",
        "passport-linkedin": "^0.1.3",
        "passport-local": "^1.0.0",
        "react": "^0.13.3",
        "serve-favicon": "^2.2.1",
        "swig": "^1.4.2",
        "validator": "^3.40.0",
        "yargs": "^3.10.0"
    }
}

@raineorshine
Copy link
Owner

The semantics in master are slightly different than in previous versions,
though the installed versions end up being the same. For example, if the
async dependency is listed as ^1.0.0, and the latest version is 1.2.1, then
the dependency does not need to be rewritten since 1.2.1 is included within
the ^1.0.0 range. That is, a normal npm update will install 1.2.1.
npm-check-updates is intended to only fill in functionality that is not
present in npm, which was confused before.

I could potentially add a flag to control this, but I’d have to be
convinced. I’m concerned that people who want the old-style behavior just don't know about npm update.

On Thu, Jun 18, 2015 at 9:27 AM, Mathieu M-Gosselin <
[email protected]> wrote:

I'm working on integrating this, but I noticed that master is only
reporting a few available updates compared to alpha 9, do you want me to
open a separate issue for that?
2.0.0 - Alpha 9:

"async" can be updated from ^1.0.0 to ^1.2.1 (Installed: 1.0.0, Latest: 1.2.1)
"babel-core" can be updated from ^5.4.7 to ^5.5.8 (Installed: 5.4.7, Latest: 5.5.8)
"body-parser" can be updated from ^1.12.4 to ^1.13.1 (Installed: 1.12.4, Latest: 1.13.1)
"compression" can be updated from 1.4.4 to 1.5.0 (Installed: 1.4.4, Latest: 1.5.0)
"csurf" can be updated from ^1.8.2 to ^1.8.3 (Installed: 1.8.2, Latest: 1.8.3)
"express-session" can be updated from ^1.11.2 to ^1.11.3 (Installed: 1.11.2, Latest: 1.11.3)
"lodash" can be updated from ^3.9.2 to ^3.9.3 (Installed: 3.9.2, Latest: 3.9.3)
"morgan" can be updated from ^1.5.3 to ^1.6.0 (Installed: 1.5.3, Latest: 1.6.0)
"serve-favicon" can be updated from ^2.2.1 to ^2.3.0 (Installed: 2.2.1, Latest: 2.3.0)
"validator" can be updated from ^3.40.0 to ^3.40.1 (Installed: 3.40.0, Latest: 3.40.1)
"yargs" can be updated from ^3.10.0 to ^3.11.0 (Installed: 3.10.0, Latest: 3.11.0)
"babel-loader" can be updated from ^5.1.3 to ^5.1.4 (Installed: 5.1.3, Latest: 5.1.4)
"bootstrap" can be updated from ^3.3.4 to ^3.3.5 (Installed: 3.3.4, Latest: 3.3.5)
"css-loader" can be updated from ^0.14.4 to ^0.15.1 (Installed: 0.14.5, Latest: 0.15.1)
"extract-text-webpack-plugin" can be updated from ^0.8.1 to ^0.8.2 (Installed: 0.8.1, Latest: 0.8.2)
"react-bootstrap" can be updated from ^0.22.6 to ^0.23.4 (Installed: 0.22.6, Latest: 0.23.4)
"react-a11y" can be updated from ^0.1.1 to ^0.2.6 (Installed: 0.1.1, Latest: 0.2.6)
"react-fa" can be updated from ^3.2.0 to ^3.3.1 (Installed: 3.2.0, Latest: 3.3.1)
"react-typeahead" can be updated from ^1.0.5 to ^1.0.8 (Installed: 1.0.5, Latest: 1.0.8)
"react-widgets" can be updated from ^2.7.0 to ^2.7.1 (Installed: 2.7.0, Latest: 2.7.1)
"spin.js" can be updated from ^2.1.0 to ^2.3.1 (Installed: 2.1.0, Latest: 2.3.1)
"webpack" can be updated from ~1.9.10 to ~1.9.11 (Installed: 1.9.10, Latest: 1.9.11)

master - :

"compression" can be updated from 1.4.4 to 1.5.0 (Installed: 1.4.4, Latest: 1.5.0)
"css-loader" can be updated from ^0.14.4 to ^0.15.1 (Installed: 0.14.5, Latest: 0.15.1)
"react-bootstrap" can be updated from ^0.22.6 to ^0.23.4 (Installed: 0.22.6, Latest: 0.23.4)
"react-a11y" can be updated from ^0.1.1 to ^0.2.6 (Installed: 0.1.1, Latest: 0.2.6)
``

package.json :

{
"name": "example",
"version": "0.0.1",
"devDependencies": {
"accounting": "^0.4.1",
"autoprefixer-loader": "^2.0.0",
"babel-loader": "^5.1.3",
"bootstrap": "^3.3.4",
"bower-webpack-plugin": "^0.1.8",
"codemirror": "^5.3.0",
"css-loader": "^0.14.4",
"exports-loader": "^0.6.2",
"expose-loader": "^0.7.0",
"extract-text-webpack-plugin": "^0.8.1",
"file-loader": "^0.8.4",
"i18next-client": "^1.9.0",
"i18next-static-analysis": "^0.0.2",
"jquery": "^2.1.4",
"jquery-form": "^3.50.0",
"json-loader": "^0.5.2",
"json5-loader": "^0.6.0",
"less": "^2.5.1",
"less-loader": "^2.2.0",
"moment": "^2.10.3",
"react-bootstrap": "^0.22.6",
"react-a11y": "^0.1.1",
"react-fa": "^3.2.0",
"react-hot-loader": "^1.2.7",
"react-typeahead": "^1.0.5",
"react-widgets": "^2.7.0",
"react-widgets-moment-localizer": "^1.0.1",
"spin.js": "^2.1.0",
"style-loader": "^0.12.3",
"summernote": "0.6.7",
"superagent": "^1.2.0",
"url-loader": "^0.5.6",
"webpack": "~1.9.10",
"webpack-dev-server": "^1.9.0",
"world-countries": "^1.7.3",
"world-currencies": "0.0.5"
},
"dependencies": {
"async": "^1.0.0",
"babel-core": "^5.4.7",
"body-parser": "^1.12.4",
"compression": "1.4.4",
"connect-flash": "0.1.x",
"connect-html-minifier": "0.1.0",
"connect-redis": "2.x",
"cookie-parser": "^1.3.5",
"csurf": "^1.8.2",
"ewait": "~0.2.1",
"express": "4.12.x",
"express-session": "^1.11.2",
"forwarded-for": "^1.0.0",
"i18next": "^1.9.0",
"json5": "^0.4.0",
"lodash": "^3.9.2",
"mailchimp-api": "^2.0.7",
"maxmind": "^0.5.5",
"morgan": "^1.5.3",
"multer": "^0.1.8",
"node-uuid": "^1.4.3",
"numeral": "^1.5.3",
"passport": "^0.2.2",
"passport-facebook": "^2.0.0",
"passport-google-oauth": "^0.2.0",
"passport-linkedin": "^0.1.3",
"passport-local": "^1.0.0",
"react": "^0.13.3",
"serve-favicon": "^2.2.1",
"swig": "^1.4.2",
"validator": "^3.40.0",
"yargs": "^3.10.0"
}
}


Reply to this email directly or view it on GitHub
#83 (comment)
.

@mathieumg
Copy link
Author

Ohh, fair enough, if it's well documented!

I'd still welcome a flag to enable the old behavior, even if semver would install 1.30 with either of ^1.0.0 and ^1.30.0, I like the explicitness of the latter. A possible use case for this is when you're curious about what dependencies in your package have updates, breaking or not.

@raineorshine
Copy link
Owner

Thanks, I’ll consider it! Check out “npm outdated —depth=0” if you haven’t
yet; does a similar thing.

On Fri, Jun 19, 2015 at 10:15 AM, Mathieu M-Gosselin <
[email protected]> wrote:

Ohh, fair enough, if it's well documented!

I'd still welcome a flag to enable the old-behavior, even if semver would
install 1.30 with either of ^1.0.0 and ^1.30.0, I like the explicitness
of the latter. A possible use case for this is when you're curious about
what dependencies in your package have updates, breaking or not.


Reply to this email directly or view it on GitHub
#83 (comment)
.

@mathieumg
Copy link
Author

Yeah I knew that command, thanks for considering! 😄

@etiktin
Copy link
Contributor

etiktin commented Sep 12, 2015

In PR #132, I added the json-parse-helpfulerror module which uses jju. So while I was working on it, I was thinking about going all in and adding support for package.json5. I didn't because I found some issues:

  • yapm also supports package.yaml. That support is not it the jju module. Instead they use an unpublished module called package-yaml and probably a couple of additional modules. Since they are not published, I'm hesitant on using them.
  • The closest-package module we use to find the closest package.json going up to the root, explicitly looks for package.json files.
  • The code in updatePackageData, uses regexp to find and update versions. We will need to change it to something that can work with all 3 formats and preserves comments. jju has an update method that might work for JSON and JSON5, but it will complicate the logic (e.g. we will need to take into account if the dependency is under dependencies or devDependencies).
  • yapm seems to have slowed down it's development (no update for about 7 months, while npm released several versions including betas for v3). If it's reaching the end of it's life, then investing in supporting it seems like a waste of time.

It's definitely possible to support this, but I think the amount of work needed, the unknown state of yapm and the limitations such a solution will inflict (e.g. we will need to replicate modules such as closest-package, and make them support the other formats) means we shouldn't do it.

What do you guys think?

@raineorshine
Copy link
Owner

Thanks for the breakdown, Eran. It sounds like significant work for minimal gain at the moment.

@mathieumg
Copy link
Author

Thanks @etiktin, this is consistent with my previous findings and what I mentioned in #94 (comment)

  • YAML support: Though I think it is interesting to have, it is more trouble to add given how different from standard JSON it is. I personally don't need it.
  • closest-package: I already forked it hastily to get it to work but I haven't yet cleaned things up and submitted a PR to that repo. Of course, ideally the PR to add the JSON5 support will get accepted in that module, but in the worst case scenario we can always use our own fork. See https:/mathieumg/closest-package/tree/variable_extension
  • updatePackageData: I have this working on alpha 9 of v2, but I have yet to rebase it on top of the latest version. See https:/mathieumg/npm-check-updates/tree/json5_support_alpha9
  • yapm: I imagine the author must be pretty busy, since he is not updating his other big project either, https:/rlidwka/sinopia. He did say he wouldn't update yapm anymore for npm 2.x, but he said he intends to release a version for npm 3.x, perhaps he's waiting for it to get out of beta. See Still maintained? rlidwka/yapm#29. If for whatever reason that 3.x fork doesn't happen, I've already started to investigate the required changes in npm to monkeypatch JSON5 support. I know I'm not alone out there who enjoys having comments in the package file (see Support for comments in package.json npm/npm#4482), it is the only reason I'm so stubborn about sticking with yapm or an equivalent.

@etiktin
Copy link
Contributor

etiktin commented Sep 14, 2015

I also like the idea of having comments in package.json, but it seems that using other formats is just too complicated and fragile. I think a simpler way to go, would be to create a separate file to hold the meta data (comments) and keep package.json as is, so it will work with the existing ecosystem.

For example, lets say I have the following package.json:

{
  "name": "ncu-tester",
  "version": "1.0.0",
  "description": "tester",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "bindings": "^1.1.0",
    "express": "^3.21.0",
    "ref": "^0.3.5"
  }
}

My package.json.meta could be something like this (using a CSON syntax):

dependencies: 'This is a single line comment'
dependencies.bindings: 'Another single line comment'
dependencies.express: '''
    This is a multi line comment
    Second line
    Third line
    '''

Then in your favorite text editor, you will have a smart plugin that can show you the preview of the meta applied on top of the json (kind of like the editors that show you a webpage preview next to the source files your editing):

{
  "name": "ncu-tester",
  "version": "1.0.0",
  "description": "tester",
  "main": "index.js",
  "license": "MIT",

  // This is a single line comment
  "dependencies": {

    // Another single line comment
    "bindings": "^1.1.0",

    /* 
    This is a multi line comment
    Second line
    Third line
    */
    "express": "^3.21.0",
    "ref": "^0.3.5"
  }
}

The plugin might even be smart enough to allow you to edit it inline and when you save, comments will be written to the meta file and keys and values will be written to the json file.
If we want to support multiple different formats for the meta file, we can agree on a convention that you have a field in the json that says what meta file to use (e.g. "metaFile": "packageMeta.cson"). The relationship between the json and it's meta, is somewhat similar to the relationship between html and css.

This way of doing things seems more sustainable to me.

@mathieumg
Copy link
Author

@etiktin I agree this is an interesting take and an innovative way to approach the issue, however it implicates editor-dependent tooling. :( An upside is that it could probably be used for many other projects who have strict JSON as their configuration file as well.

It would need to handle cases where a/many valid JSON line is/are commented out, as well. In such cases it would need to remove the line(s) from the JSON file and add it/them as a comment to the appropriate sibling node in the meta file, and vice-versa. It would need to handle these cases when there are no siblings as well, possibly due to the same operation.

There's no denying it would be much more sustainable than forking or monkey patching the package manager.

@raineorshine
Copy link
Owner

Closing for cleanup purposes and adding the revive-me label. Feel free to re-open if you are interested in working on a pull request.

@mathieumg
Copy link
Author

I was waiting on hughsk/closest-package#2

I guess I'll just create my fork.

@raineorshine
Copy link
Owner

Sure. Let me know if you're actively working on it and I'll re-open.

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

No branches or pull requests

3 participants