Skip to content

fuse bsconfig into package.json #5455

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

Conversation

aspeddro
Copy link
Contributor

This PR is a proposal to read dependencies, dev dependencies and name from package.json

@aspeddro
Copy link
Contributor Author

Need to resolve pinned dependencies. They are currently specified in bs-dependencies. Is workspaces in package.json equivalent?

@Minnozz
Copy link
Contributor

Minnozz commented Jun 28, 2022

Need to resolve pinned dependencies. They are currently specified in bs-dependencies. Is workspaces in package.json equivalent?

See this repository (note the branch) for a minimal example of workspaces: https://github.com/Minnozz/rescript-reproduce-issue/tree/pinned-dependencies-issue

@aspeddro aspeddro marked this pull request as draft June 28, 2022 20:08
@aspeddro aspeddro force-pushed the feat-bsconfig-into-packagejson branch from 0a99ed4 to d7fd9d8 Compare June 28, 2022 22:27
@aspeddro
Copy link
Contributor Author

Need to resolve pinned dependencies. They are currently specified in bs-dependencies. Is workspaces in package.json equivalent?

See this repository (note the branch) for a minimal example of workspaces: https://github.com/Minnozz/rescript-reproduce-issue/tree/pinned-dependencies-issue

thank you @Minnozz

An example of monorepo https://github.com/aspeddro/rescript-monorepo-experiments/tree/monorepo-test-take-one. The pinned-dependencies are defined in package.json

@cristianoc
Copy link
Collaborator

Could you explain the proposal at high level?
This is still draft, so maybe in flow. But seems to introduce a breaking change to existing projects, while one would expect purely additive functionality.

@aspeddro
Copy link
Contributor Author

Problem

  • Currently dependencies and dev dependencies are added to package.json and bsconfig.json
  • The name should be the same in bsconfig.json and package.json

This information is already in package.json.

Solution

Read package.json, iterate over the dependencies and devDependencies and check if a bsconfig.json exists.

This approach implies a loss of performance if the package.json is large, but I believe that people are willing to pay this cost in exchange for this ergonomics.

One caveat, I'm exploring the compiler, I might be breaking some rule.

Related: #5278

@aspeddro
Copy link
Contributor Author

aspeddro commented Jun 29, 2022

But seems to introduce a breaking change to existing projects, while one would expect purely additive functionality.

If I keep pinned-dependencies in bsconfig.json will it break for existing projects?

@cristianoc
Copy link
Collaborator

But seems to introduce a breaking change to existing projects, while one would expect purely additive functionality.

If I keep pinned-dependencies in bsconfig.json will it break for existing projects?

I guess the question is: is it true that any project correctly configured today will still work exactly the same after this change?

Also, I guess bsconfig allows to specify a subset of the dependencies defined in package.json. If so, is there still a way to do so. Should there be?

If one can do zero breaking changes, that's ideal.
If some breaking changes are unavoidable, there's still the pending thing of using rescript.json, and one can consider to introduce the change at the same time as the rename.

@aspeddro
Copy link
Contributor Author

I guess the question is: is it true that any project correctly configured today will still work exactly the same after this change?

No, after this change package.json is required.

One option to maintain compatibility is to keep name in bsconfig.json.

@cristianoc
Copy link
Collaborator

I guess the question is: is it true that any project correctly configured today will still work exactly the same after this change?

No, after this change package.json is required.

One option to maintain compatibility is to keep name in bsconfig.json.

Can you expand a bit on the explanation, even though it might seem obvious. And with a concrete example?

@aspeddro
Copy link
Contributor Author

Sorry for delay.

Here an valid example build_tests/react-ppx that will fail.

  • Compile search for package.json to read name field, but file is not found.

@cristianoc
Copy link
Collaborator

Sorry for delay.

Here an valid example build_tests/react-ppx that will fail.

  • Compile search for package.json to read name field, but file is not found.

I suggest to turn this into a proposal with no breaking changes, give a description which is a bit more detailed than the one above (with examples) and try to reach consensus on the forum.

@cknitt
Copy link
Member

cknitt commented May 26, 2024

@aspeddro Can this one be closed?

(We'll want to revisit configuration after rewatch has been fully integrated.)

@aspeddro aspeddro closed this May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants