devconfig: allow comments and trailing commas

[gcurtis]

This allows comments and trailing comments in devbox.json as supported by hujson/JWCC.

// Top-level comment about the config.
{
	"packages": {
		"go": "1.20",
		
		// Needed to decompress stuff on macOS
		"xz": {
			"version": "latest",
			"platforms": ["aarch64-darwin"],
		},
	},
	"shell": {
		"scripts": {
			/*
			A longer block comment about some build script.
			*/
			"build": "go install ./..",
		},
	},
}

As a part of this change, the way the add/rm commands edit and marshal devbox.json has changed significantly. We can no longer overwrite the previous JSON, since that would lose the user's comments and formatting. There's a new configAST type to handle surgical edits to the JSON so that those things aren't lost.

Unmarshalling the config is still done the same way. The docs in ast.go have more details.

[mikeland73]

I think this is really interesting, but the complexity is a big concern.

//   - Marshalling is done by calling configAST.root.Pack to encode the AST as
//     hujson/JWCC. Therefore, any changes to a Config struct will NOT
//     automatically be marshaled back to JSON. Support for modifying a part of
//     the JSON must be explicitly implemented in configAST.
//   - Validation with the AST is complex, so it doesn't do any. It will happily
//     append duplicate object keys and panic on invalid types. The higher-level
//     Config type is responsible for tracking state and making valid edits to
//     the AST.

Does this mean every new field we want to add to devbox.json needs custom handling in configAST?

If yes, that would be a really big tradeoff in terms of amount of work and maintainability.

[gcurtis]

@mikeland73

Does this mean every new field we want to add to devbox.json needs custom handling in configAST?

Only for fields that are edited by the CLI. Fields that are only read by the CLI and edited by the user don't need to go in the AST. In other words, the CLI can't just set a struct field to a new value:

if cfg.SomeField == "foo" {
    cfg.SomeField = "new"
}
b := cfg.Bytes() // SomeField will still be "foo"

There's a lot of logic that would be simpler with consistent typing. Much of the code comes from converting packages between an array of strings, a map of strings, and a map of objects while preserving comments/formatting/member order. Other than that, most of it is just boilerplate that's creating or setting JSON values.

I think this feature is worthwhile. We've had a lot of users ask for it, and it's still much simpler than tyson.

[savil]

Reviewed everything except the ast.go. So far, this is looking great to me! Will review that shortly after a break.

[savil]

This lgtm! I think I can live with some of the increased overhead of needing to some golang code for fields we have the CLI code write to. Do check in with @mikeland73 if his concerns are blocking...

That said, I also advocate for us shipping a change that migrates all users to the map-style packages when they do devbox update. That will help reduce the size of code in ast.go, and the test-cases, and general complexity. Note, we should plan this a bit so that docs can also be appropriately updated at the right time.

[mikeland73]

I think this feature is worthwhile. We've had a lot of users ask for it, and it's still much simpler than tyson.

FWIW, I don't see this competing with the tyson implementation in #1555
That PR is tightly scoped so it does not affect users that don't use tyson. It is also very small in terms of code and does not import tyson as a golang dependency at all.

Functionally they are different as well. The tyson PR is mostly to augment devbox.json, not replace it.

My concern with this PR is that it replaces a straightforward JSON marshalling with a custom marshaller/unmarshaller. This means we have to maintain a custom marshaller/unmarshaller forever. The cost-benefit of a decision like that needs to be very clear because this would add new overhead to config development and this would be a pretty painful change to revert.

[mikeland73]

I'm not a hard no on this PR but would like to chat about it before merging. I also want @loreto's opinion.

[gcurtis]

FWIW, I don't see this competing with the tyson implementation in #1555

I don’t see it as competing with any PRs either. I’m just saying that out of the discussed potential changes to devbox configuration, this is about as minimal as it gets.

I’d really like to find a way to allow comments in devbox.json. I’m open to ideas, but I don’t see a simpler way to do that that’s also backwards compatible.

[loreto]

I'm not a hard no on this PR but would like to chat about it before merging. I also want @loreto's opinion.

Are you guys asking me to comment on the feature (allowing comments and trailing commas), or on the implementation?

I'm a yes on the feature itself, but I can also take a look at the implementation if that's what you want me to take a look at.

[mikeland73]

Concept Ack. Nice work!

[weilbith]

Hey 👋🏾
How I do use now comments? And how do I convince my editor that this is fine within JSON unless it is not JSON-C? 🤔

[gcurtis]

@weilbith you can add // comment and /* comment */ style comments anywhere in devbox.json as long as you're using a newer Devbox release.

If your editor allows you to override the syntax, you can tell it to use jsonc or json5. I know in VSCode you can do this by adding a file association to settings.json. For example:

"files.associations": {
  "devbox.json": "jsonc"
}

[weilbith]

Quick question. Not only regards of comments, readability (?) but also multi line scripts/init_hooks, would YAML be very easy to be supported? I know only very basic Go, but shouldn't most serialization libraries make that a no-brainer? Sorry, just curious. 🙈