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

Remove refmt.exe / Reason syntax support #5683

Merged
merged 9 commits into from
Sep 24, 2022

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Sep 22, 2022

We'll finally be removing support for legacy Reason syntax in ReScript 11.

Existing projects using Reason syntax can be converted to ReScript syntax using the rescript convert command in ReScript 9.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's also some stuff that is used for detecting dependencies between files, i.e. in the builder or somewhere. Not super important, but it becomes essentially dead code.
Let me try to find it.

@@ -16,6 +16,5 @@
"warnings": {
"error" : "+101"
},
"namespace": true,
"refmt": 3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to this config option? I think it should just be ignored for now -- no need to introduce a breaking change.

Copy link
Member Author

@cknitt cknitt Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was already ignored and could safely be removed? I don't have this setting in any of the bsconfig.json files in our projects.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I did not mean to imply that it is not ignored. It was just a thought, to check that it is indeed the case.

@cristianoc
Copy link
Collaborator

Looking in literals.ml and starting removing:

let suffix_re = ".re"

let suffix_rei = ".rei"

should get rid of a bunch of dead code.
Can also be done in a separate PR bwt.

@cknitt cknitt marked this pull request as ready for review September 22, 2022 15:44
@cknitt cknitt marked this pull request as draft September 23, 2022 14:07
@cristianoc
Copy link
Collaborator

I guess this requires a good entry in the changelog, including how to use an older version of the compiler to convert legacy Reason projects.

@cknitt cknitt marked this pull request as ready for review September 24, 2022 09:28
@cknitt cknitt merged commit f25a373 into rescript-lang:master Sep 24, 2022
@cknitt cknitt deleted the remove-refmt branch September 24, 2022 11:20
cristianoc pushed a commit that referenced this pull request Oct 1, 2022
* Remove bsrefmt

* Remove refmt.exe

* Remove "refmt": 3

* Remove ast_reason_pp module and re/rei file extensions

* Remove leftover js_refmt_compiler

* Remove refmt_main3.ml

* Fix moveArtifacts script

* Clean up literals.ml

* Update CHANGELOG
@danielo515
Copy link

Why? If people are willing to maintain a converter between reason and rescript why force it?

@ryyppy
Copy link
Member

ryyppy commented Oct 7, 2022

@danielo515 It is more than two years where we announced our plans to put our focus on a homogenous ecosystem with one curated, well supported syntax (which is ReScript). All our infrastructure (including editor-support) is relying on .res files and we provide all the tools to migrate from .re to .res OOTB as well.

We decided that we don't have the capacities to continue this maintenance effort, and it's limiting us greatly in improving our build system toolchain and allowing easier support for ARM based binaries (which will definitely be way more important to our core audience than supporting .re files). refmt also made things way bulkier... e.g. the playground JS bundle doubles in size due to the vendored Reason source code (the RE syntax is heavier than the compiler + ReScript syntax together due to the way its vendored). The refmt.exe only works on MacOS M1 because of Rosetta, and I believe it doesn't even work on Linux ARM.

So it's mostly an economic question of what we want to spend our time on, how we can make our development effort more streamlined, and most importantly, how to get the most bang for our bucks. This is one huge stepping stone to get there.

Since this is a major version release, we don't expect anyone to upgrade right away, so there's enough time to upgrade all the libraries that are still running on .re syntax anyways.

@danielo515
Copy link

danielo515 commented Oct 11, 2022 via email

cristianoc pushed a commit to d4h0/rescript-compiler that referenced this pull request Oct 27, 2022
* Remove bsrefmt

* Remove refmt.exe

* Remove "refmt": 3

* Remove ast_reason_pp module and re/rei file extensions

* Remove leftover js_refmt_compiler

* Remove refmt_main3.ml

* Fix moveArtifacts script

* Clean up literals.ml

* Update CHANGELOG
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