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

Migrating to ReScript #41

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Migrating to ReScript #41

wants to merge 5 commits into from

Conversation

diogomqbm
Copy link

Migrated everything to rescript and changed reason-react to @rescript/react

@andywhite37
Copy link
Member

Hey @diogomqbm - thanks for making this pull request! We haven't really considered what migrating to rescript means for all these libraries, so I'm not sure what the implications will be for this library, the relude ecosystem as a whole, and users of these libraries. I think I'd like to see if anyone has any feedback on this before we consider merging it.

@johnhaley81
Copy link

We (Qwick) don't use relude-reason-react (however it looks fantastic and we probably should) but we (Qwick) don't have any plans to migrate to ReScript for the foreseeable future. However, if it doesn't break the relude ecosystem I don't see any reason not to do it.

@diogomqbm
Copy link
Author

diogomqbm commented Oct 8, 2021

So, we're using relude and relude-reason-react with ReScript at Walnut and it works just fine. The thing here is that there's no reason for us to have dependencies like reason-react since we've migrated to ReScript and @rescript/react. However, I don't know how this can impact those using melange/reason-react such as @johnhaley81. This can be a breaking change for the reason ecosystem.

@diogomqbm
Copy link
Author

diogomqbm commented Oct 8, 2021

I would appreciate it if we had something like relude-rescript-react.

@mlms13
Copy link
Member

mlms13 commented Oct 8, 2021

It does indeed seem like the React-specific side of the ecosystem is going to exist primarily with Rescript, and as this is a React-specific library, it doesn't make sense for us to continue to depend on the outdated reason-react package.

A couple thoughts:

  • Could we first change our dependency to @rescript/react without migrating the rest of the code to Rescript? I'm not against migrating this project to Rescript if we don't lose anything major in the process, but I'm worried that changing the underlying peer dependency and migrating all of this code is a lot of moving parts.
  • For the Rescript migration, I wonder if git mv would help better track the diffs across the file renames. Right now the diff is a bit hard to read because it looks like git didn't understand some of the renames.
  • Removing the npm run clean script seems to have broken the codecoverage step in CI. Maybe there's no need to clean, or maybe rescript has something like bsb's clean

@diogomqbm
Copy link
Author

diogomqbm commented Oct 8, 2021

So, I don't know if just changing the dependency is going to work. I don't know how ReScript to Reason works. The only thing that I needed to drastically change was removing __tests__/ReludeReact_Reducer_test.re since it had a dependency on bs-react-testing-library which also had reason-react and then it started to conflict with @rescript/react. I also tried to convert bs-react-testing-library to ReScript but things are more complicated there, it's throwing a PPX error when changing to rescript.

The rest was pretty straightforward following the migration guide and fixing the warnings.

@jihchi
Copy link
Contributor

jihchi commented Oct 9, 2021

Regrading bs-react-testing-library, I would suggest that we find an alternative and move on.

@diogomqbm
Copy link
Author

Regrading bs-react-testing-library, I would suggest that we find an alternative and move on.

@jihchi Sorry for the delay, it's been a crazy month. Just wrote some internal bindings that will do the job.

@diogomqbm
Copy link
Author

  • Removing the npm run clean script seems to have broken the codecoverage step in CI. Maybe there's no need to clean, or maybe rescript has something like bsb's clean

sorry about that, I just added it again

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.

5 participants