Don't error on comments & trailing commas in package.json #10287
Conversation
|
❌ @Jarred-Sumner 3 files with test failures on linux-x64-baseline: |
|
❌ @Jarred-Sumner 3 files with test failures on linux-x64: |
|
❌ @Jarred-Sumner 4 files with test failures on bun-darwin-aarch64:
|
|
❌ @Jarred-Sumner 11 files with test failures on bun-windows-x86_64-haswell
|
|
❌ @Jarred-Sumner 4 files with test failures on bun-darwin-x64:
|
|
Maybe consider adding a warning in the console, since the feature is not recommended and is not spec compliant.
|
|
Have you considered tooling that reads a user's It's probably reasonable to move to a dynamic |
| }, | ||
| // This is a fast pass I guess | ||
| 2 => { | ||
| if (strings.eqlComptime(source.contents[0..1], "\"\"") or strings.eqlComptime(source.contents[0..1], "''")) { |
There was a problem hiding this comment.
[0..2] for all of these
| }, | ||
| }); | ||
| // itBundled("packagejson/SyntaxErrorComment", { |
There was a problem hiding this comment.
should we just delete these?
There was a problem hiding this comment.
yeah, will do in a follow-up
|
Consider the following package.json. {
"devDependencies": {
"@company-repo/prettier-config": "1.0.0",
// We can't use the new version until the prettier config supports it.
"prettier": "2.8.8"
},
"name": "@company-repo/my-package",
"prettier": "@company-repo/prettier-config",
"scripts": {
"format": "prettier --write ."
}
}As it is now, You can probably remember something like this playing out when Bun hit 1.0, where something about Bun's implementation meant certain packages didn't work, and users would file issues with the repo for those packages. It wasn't what Bun intended, users were instructed to file issues with Bun instead, but it still happened because that's the conclusion some users are going to come to. The difference here is that it's not a bug, it's a feature. The behavior that causes the script to crash will continue, because there's nothing for Bun to fix or implement that will make it stop. So users will keep running into this, not just with Prettier but whenever they try to use this feature along with a package that doesn't parse JSON with comments, and some of them will continue to come to the conclusion that it is an issue with those packages. You could say that Prettier could avoid this by allowing package.json files to have comments, which would probably be easier for it to do that most packages since, like Bun, it already has code to handle JSON with comments, but that just sort of spreads the problem. Because now it's "okay, Bun and Prettier allow this, but such-and-such doesn't, so I should file an issue with such-and-such", and now such-and-such has to either put effort into getting config to read from JSON with comments or just put up with the extra noise on the issue tracker. You can say
but please remember the words of Tony Hoare, "I couldn't resist the temptation to put in a null reference, simply because it was so easy to implement." The trouble this change will cause will be nowhere near the magnitude of issues stemming from that, but it is not nothing, either. It will add friction, going from Node to Bun or back again. It will add tension between Bun and package maintainers. |
What does this PR do?
This makes
bun install,bun run <script>,bun ./<file>,bun build <file>no longer error when package.json contains trailing commas, single-line comments or multi-line comments.We do not recommend this feature, as it will break usage in other tools (as well as
JSON.parse). But enough people have asked for it and it's very easy to add. We trust people can make their own decision whether or not it's worth the tradeoffs.Fixes #9036
How did you verify your code works?
There are tests