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
[rush] [heft] Allow for config files to have jsonc
extension(s) when json files contain comments. (feat proposal)
#3615
Comments
That would be useful. We'd have to handle the case when both a Would you be interested in putting this together? |
@iclanton I'm not sure that I agree with this design. If we allow a mixture of
If Perhaps we could:
|
We'd probably be better served switching to YAML, since it handles Git merges a lot better. Merge conflicts in JSON files because you modified config settings on adjacent lines get old fast. |
YAML has some downsides:
This problem is particularly annoying in the JSON5 allows trailing commas, which I think would solve this, right? |
@iclanton @octogonz I could implement it but it is a huge change and think a discussion (such as this one) needs to happen first... I think the default is "if jsonc file exists, use that, but if missing, use json, and throw error if both exist"; but this doesnt really handle the possible doubling of io requests to the file-system... I think the cleanest solution is an "opt-in-use-jsonc-config-setting" and/or a @dmichon-msft (what pete said) and my own thoughts:
|
food for thought: I wrote an auto-installer and global command to use |
@iclanton @D4N14L @apostolisms @elliot-nelson We seem to have gotten quite a lot of feedback about this topic in recent months (via a number of different tangents that start with "I opened a Any change to the Rush Stack config files is going to be a nontrivial undertaking, and maybe not our top priority. HOWEVER that's separate from deciding a position. Are we going to maintain the previous stance, that As a "high" priority: Let's weigh the options and try to reach a consensus, and then we can share that as our official position/goal. That will avoid wasting resources by revisiting this topic endlessly. 😄 |
Well, the good news is that as soon as you start searching for 2 files, searching for 20 is the same number of API calls, since at that point you should be querying for a directory listing instead of doing individual existence checks. On the other hand, I am very much in favor of predictable configuration file locations. For the record, I honestly had no idea YAML had so much other stuff in it (the variable expansion stuff I really didn't expect); I've only ever used it as "expanded comment-friendly merge-friendly JSON", e.g. pnpm-lock.yaml and Azure Pipelines YAML. I know that the reason we don't like |
I thinks worth mentioning that adding |
Executable code cannot be cached reliably, because we can't be sure that the expressions have no side effects. (For example, consider all the ways that PNPM fails to detect certain kinds of changes from A It can't be loaded at all by a different programming language. Loading is equivalent to implementing a JavaScript interpreter. A There are sooo many downsides... :) |
I do not know why my EditI have fixed the problem for myself in the workspace {
// to fix an issue in the RushJS config files
"files.associations": {
"*.json": "jsonc"
}
} |
@Luxcium that works, but in the intellij ides you cannot set such a file-association and have a |
Ah, I don't think I understood the issue before. In IntelliJ, the presence of a And, it's probably not fixable on the schema side, because a schema can't say a file is "json with comments", that's not a concept that exists in the specification. |
@elliot-nelson the problem is fix-able by having a |
Summary
Editor setup and syntax highlighting would be nicer and easier (in my opinion) if heft and rush config files were allowed to have
jsonc
as their file extension.Several members of my team us webstorm/intellij and a slew of other editors for typescript, many of which hare not as easy to configure as vscode to handle file associations based on paths :/
The text was updated successfully, but these errors were encountered: