Skip to content

[RFC] Allow tyson config #1555

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

Closed
wants to merge 2 commits into from
Closed

[RFC] Allow tyson config #1555

wants to merge 2 commits into from

Conversation

mikeland73
Copy link
Contributor

Summary

This allows tyson config. It uses runx so the binary does not statically depend on tyson.

TODO: Need to scope tyson to only certain commands that don't modify config (otherwise we end up writing the mutations to the config).

How was it tested?

 devbox git:(landau/tyson-rc) devbox run tyson-script        
hello TySON world

@gcurtis
Copy link
Collaborator

gcurtis commented Oct 12, 2023

My vote would be to hold off on tyson. I think we should be using the same devbox.json that our users are using, and introducing a programming language + runtime for config parsing is a huge change.

If we need this functionality internally, could we just add the typescript package and run scripts through the init hook? Or even install tyson as a regular package through runx without modifying devbox?

@mikeland73
Copy link
Contributor Author

@gcurtis the specific use case that is interesting for us is turning package.json scripts into devbox scripts. We end up essentially copy pasting them. I'm not sure how we would solve that without something like this. I guess we could write a script that syncs them, but that's a bit manual.

This change as implemented would not replace devbox.json for us, it would simply mutate it for consumption purposes. Specifically tson would not be used to add standard packages, scripts, or anything else that is doable in json.

I'm actually not totally sold on having tyson have it's own runtime. For example we could implement something similar by just using node on the host machine or even node as installed by devbox/nix. The approach in this PR was specifically chosen to avoid including tyson as a dependency. This approach is as opt-in as I could make it and should have no risk or effect for users that don't which to use it.

@Lagoja
Copy link
Contributor

Lagoja commented Oct 13, 2023

I'm not sure we should merge or add TySON support unless we want to support it fully, or it unlocks some benefits that we want to leverage. Merging scripts from the package.json seems like a JS/TS specific benefit, and probably not significant enough to justify adding the feature.

Even if we're just using TySON as a mutator for the Devbox JSON, it seems like it would add complexity to managing a project's configs.

@mikeland73
Copy link
Contributor Author

it unlocks some benefits that we want to leverage

The package.json example is a benefit. Another benefit is we can run pnpm install conditionally which will speed up running scripts. Having not to write everything in bash is a pretty big benefit.

Merging scripts from the package.json seems like a JS/TS specific benefit, and probably not significant enough to justify adding the feature.

There's nothing JS/TS specific about this example. Any other language/package manager with scripting (e.g. poetry) will have similar issues that can be solved via TySON. The concept is universal: transform scripts from framework X into devbox scripts.

Even if we're just using TySON as a mutator for the Devbox JSON, it seems like it would add complexity to managing a project's configs.

I'm not sure I understand/agree. If TySON is only evaluated if a tson file exists, this strictly limits the scope/risks of this change. Devbox continues to write only to devbox.json.

@mikeland73
Copy link
Contributor Author

@loreto and @gcurtis thoughts on reconsidering this PR?

In the font-end repo we have package.json and devbox.json for each project and maintaining that is a pain. This change would allow us to make devbox.json mimic the package.json functionality without requiring that we copy paste every command.

I'm OK putting this behind a feature flag.

Just to reiterate,

  • this PR does not pull in any tyson code or dependency. It simply installs the tyson binary only if the user opts in. So the risk is close to zero. If we add a feature flag the risk is even lower.
  • This PR does not change the format of devbox.json (response to [RFC] Allow tyson config #1555 (comment))
  • This PR is not specific to js/ts, it just happens that the problem I'm currently trying to solve involves a typescript stack. (response to [RFC] Allow tyson config #1555 (comment))

@mikeland73 mikeland73 closed this Dec 7, 2023
@mikeland73 mikeland73 deleted the landau/tyson-rc branch December 7, 2023 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants