[BUG] Comment stripping for JSON in node 19 doesn't work about node-config HOT 23 OPEN

I'm ok with that solution. Doubt many would care about the extra strictness of JSON vs. JSON5, although it wouldn't surprise me if someone commented about it 😄

from node-config.

I'm not sure what changed here. PR welcome.

from node-config.

@edomora97 I ran into the same issue. It looks like JSON.parse tries to be more helpful and has 3+ error conditions when trying to use comments in JSON files in Node 19.

One workaround is to use the .json5 extension

from node-config.

I'd like to support comments in JSON with Node 19 if someone wants to figure out the patch.

from node-config.

Error message of JSON.parse has been changed.

default.json:

{
  /*comments*/
}

Node 16.14.2:
e.message = 'Unexpected token / in JSON at position 5'

Node 19.9.0:
e.message = 'Expected property name or '}' in JSON at position 5'

Culprit:

from node-config.

suggest to always fallback to json5 when there's an syntax error (since you already support the fallback

from node-config.

Is this also a problem with node 20? (I saw this with node 19 and rolled back)

from node-config.

Yes

from node-config.

We are blocked from moving to node 20 because of this change in behaviour. Is the PR going to be merged or other solution?

from node-config.

Something new on this issue?

from node-config.

I just took a look at the history here. Here's what I found:

• Prior to 1.18.0, we supported comments in JSON, but there was a bug, so it could mis-parsed.
• Discussion ensued in #256. The principled approach was discuss of leaving "JSON as JSON". With a breaking change in a major release, we would drop support for comments in JSON and tell folks to use JSON5 instead, which support that as part of the spec.
• But there was pushback to keep backwards compatibility, resulting in the behavior we had today: We would parse JSON without stripping comments, but if the error message when it failed indicated that the problem was related to comment parsing, in that case we would fallback to JSON5 parsing.

The proposal here is to /always/ fallback to JSON5 parsing if JSON parsing fails. This effectively means that all JSON5 features become valid in JSON fails, which was never the intent.

This was something at the author, @lorenwest argued against then, and I agree that's not a great resolution.

Now, let's pivot to update from the JSON5 project to see what's happening with it:

JSON5 is an extension to the popular JSON file format that aims to be easier to write and maintain by hand (e.g. for config files). It is not intended to be used for machine-to-machine communication. (Keep using JSON or other file formats for that. 🙂)

JSON5 was started in 2012, and as of 2022, now gets >65M downloads/week, ranks in the top 0.1% of the most depended-upon packages on npm, and has been adopted by major projects like Chromium, Next.js, Babel, Retool, WebStorm, and more. It's also natively supported on [Apple platforms]
(https://developer.apple.com/documentation/foundation/jsondecoder/3766916-allowsjson5) like MacOS and iOS.

Rather than fade away, this JSON superset has gone on to to see wide adoption and endorsement from major projects, precisely because it's good for cases like ours--config files

So at this point, I propose instead of this approach, we completely drop the use of the regular JSON parser, and always use the JSON5 parser. This would be backwards compatible for all users, and is a rare case where we can both add a feature and simplify the code.

If @lorenwest doesn't comment in >1 week, I'll proceed with that plan. In the meantime, if someone wants to move the proposal along, they could add a PR for it, and make sure we have test coverage. Our docs should be updated now and the wiki after the release.

from node-config.

Anyone who needs an immediate workaround and rename their JSON config files to .json5 and make sure to the JSON5 module installed. That will trigger our JSON5 parser code path, where comments in JSON have always been supported.

from node-config.

Thanks. PR is welcome then to implement the above.

from node-config.

Fixing the test failure described here would also be welcome:

#704 (comment)

from node-config.

This issue also affects bun runtime as well as node 19+

from node-config.

This issue also affects bun runtime as well as node 19+

When using node-config via the config/default.ts directory, the error below occurs when running bun build/src/app.js

[0.41ms] ".env"
848 |   }
849 |   catch (e3) {
850 |     if (gitCryptTestRegex.test(fileContent)) {
851 |       console.error('ERROR: ' + fullFilename + ' is a git-crypt file and CONFIG_SKIP_GITCRYPT is not set.');
852 |     }
853 |     throw new Error("Cannot parse config file: '" + fullFilename + "': " + e3);
              ^
error: Cannot parse config file: '$PATH/config/default.ts': TypeError: module_1.Module._preloadModules is not a function. 
(In 'module_1.Module._preloadModules(service.options.require)', 'module_1.Module._preloadModules' is undefined)

 at /node_modules/config/lib/config.js:853:10
  at /node_modules/config/lib/config.js:649:20
  at forEach (:1:20)
  at /node_modules/config/lib/config.js:648:2
  at new Config (/node_modules/config/lib/config.js:113:21)
  at /node_modules/config/lib/config.js:1510:30
  at globalThis (/node_modules/config/lib/config.js:1520:101)
  at require (:1:20)
  at /build/src/app.js:7:17
  at globalThis (/build/src/app.js:14:1)

This can be recreated in any Express app that uses node-config and the Bun environment

Contents of config/default.ts

interface AppConfig {
  port: number
}

const config: AppConfig = {
  port: 8080
}

export default config

I also have a config/custom-environment-variables.ts which references from the .env file

export default {
  port: 'PORT',
  dbUri: 'DB_URI'
}

The app works fine in Node 18 when running node build/src/app.js

from node-config.

Hello 😄 ,
I have been looking into both the parsing issue & #704.
As for the JSON parsing error for invalid token '}', it seemed to be the matter of a RegExp issue. It was previously matching

So I changed it to the following and the tests started passing again.

  stringRegex = stringRegex || /(['"])(\\\1|[a-z0-9 _\/\{\}\?\=\<\>\:\.]|)+?\1/gi;

I will continue investigating the #704 error, so far nothing groundbreaking, the only thing I noticed is the actual object returned from CONFIG.get('aPromise') is a proxied Promise.

from node-config.