Skip to content
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

Support rescript.json #6193

Closed
wants to merge 15 commits into from
Closed

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Apr 23, 2023

Resolves #5278

  • Introduce a new compiler flag -p which means "project". The location of the rescript.json (or bsconfig.json) file allows user to specify the project path where the build/format/etc will start.
    Don't introduce any new flags. This will be discussed separately if needed.
  • The binary determines wether to use rescript.json or bsconfig.json. rescript.json precedence over bsconfig.json.
  • When walking dependencies, rescript.json is tried first. Ignores bsconfig.json if present.

@cometkim cometkim force-pushed the new-config branch 2 times, most recently from 8680c09 to 5a33236 Compare May 1, 2023 07:13
@cristianoc
Copy link
Collaborator

Quick Q: what's the rationale to rename the concept of config to manifest?

@cometkim
Copy link
Member Author

cometkim commented May 17, 2023

Quick Q: what's the rationale to rename the concept of config to manifest?

It doesn't rename config to manifest, it's a separate concept the part dealing with user manifest as data.

manifest logic is cheap and can be shared across multiple modules. config logic is heavy and can be derived from manifest and system state.

@cristianoc
Copy link
Collaborator

Quick Q: what's the rationale to rename the concept of config to manifest?

It doesn't rename config to manifest, it's a separate concept the part dealing with user manifest as data.

manifest logic is cheap and can be shared across multiple modules. config logic is heavy and can be derived from manifest and system state.

Not sure I understand the answer -- but sounds like you've thought about it, so I might ask more later on during review.

@cometkim
Copy link
Member Author

cometkim commented May 17, 2023

I'm cleaning up the heavy logic of config. It's not pure and hard to compose because it uses a lot of system calls while parsing the configuration simultaneously.

I extract the pure part (almost json parsing) from there and give it the name manifest.
So the flow would be json -> manifest -> bsb_config.

bsb_config is for bsb, but the manifest can be easily shared by other modules like syntax, gentype, etc.


This is not a good time to review because I haven't removed the redundant logic in bsb_config yet.

@fhammerschmidt
Copy link
Member

What's the status of this? Would love to have this merged so that I can implement support for it in rewatch as well.

Furthermore, rescript.json should be an opportunity to clean up the config.

Personally, I would remove the following elements:

  • reason
  • entries
  • pp-flags (think those would only work ml files?)

Also, I would rename the remaining fields that contain "bs":

  • bs-dependencies --> dependencies
  • bs-dev-dependencies --> dev-dependencies
  • bsc-flags --> compiler-flags
  • bs-external-includes --> external-includes (or remove that as well)

And the source item has a lot of attributes that are experiments or even don't work at all (e.g. resources, group)

@cometkim
Copy link
Member Author

Furthermore, rescript.json should be an opportunity to clean up the config.

I think so. also as you mentioned, each field setting has too many variants so I want to clean this up too.

The goal of this PR is to support all existing fields for now. Others will be follow-ups.

The refactoring to separate config and manifest should make it easier and to use manifest parsing for third-party tools as well.


I've taken a break from OSS work in recent months. Will revisit soon, hopefully in this month.

@fhammerschmidt
Copy link
Member

fhammerschmidt commented Aug 17, 2023

Ah yes. In fact even the current state of this PR could be split up into multiple.

  1. support for rescript.json just the file with the same contents + deprecation warning for bsconfig
  2. the missing fields (gentype, ...)
  3. the split of the config into manifest and config

No 1. should be relatively straightforward?

@cometkim
Copy link
Member Author

No 1. should be relatively straightforward?

Yes, I can make it since I remember the steps for. I'll make a clean PR for it this weekend.

@DZakh
Copy link
Contributor

DZakh commented Aug 18, 2023

I've just thought that it would be nice to automatically convert bsconfig.json to rescript.json when user runs a compiler and it detects bsconfig.json.

@cometkim cometkim mentioned this pull request Sep 4, 2023
@cometkim cometkim closed this Oct 3, 2023
@cometkim cometkim deleted the new-config branch November 1, 2023 16:31
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.

support both bsconfig.json and rescript.json
4 participants