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

Migrate to rescript #75

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

Conversation

amiralies
Copy link

closes #74

What does this PR do:

  • Upgrade compiler version
  • Convert to rescript syntax
  • Use rescript react instead of reason react
  • Remove legacy parts
  • Fix incomatibilties in upgrade process
  • Fix some tests

Copy link

@Minnozz Minnozz left a comment

Choose a reason for hiding this comment

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

Awesome, I was just starting to look into using Reductive with ReScript.

I noticed some converted code that might not be idiomatic / officially supported in ReScript. Also, the README still contains references to Reason.

| B => state ++ "b"
}

type ReduxThunk.thunk<_> +=
Copy link

Choose a reason for hiding this comment

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

Not sure if this is idiomatic / officially supported in ReScript: rescript-association/rescript-lang.org#135

Copy link
Author

Choose a reason for hiding this comment

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

I think this is supported (talking about current state) but not documented just like row polymorphism and other advanced stuff.
but definitely examples need some improvement.

}

let thunkedLoggedTimeTravelLogger = (store, next) =>
\"@@"(Middleware.thunk(store), \"@@"(Middleware.logger(store), \"@@"(timeTravel(store), next)))
Copy link

Choose a reason for hiding this comment

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

I don't think the @@ operator officially exists in ReScript. It is not listed here.

Copy link
Author

Choose a reason for hiding this comment

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

yeah the application operator is not supported.

@Minnozz
Copy link

Minnozz commented Sep 6, 2021

@amiralies Do you intend to continue working on this PR? Can you use help?

@amiralies
Copy link
Author

@Minnozz I can address the issues but I'm curious if ricky @rickyvetter willing to continue maintaining this since I didn't get response from him

@stevez
Copy link

stevez commented Mar 28, 2022

@amiralies, I like your pr, today I build you branch on my machine and found it build successfully! but I can't build successfully in the master branch. But when I run the example of todomvcEntry.res, it doesn't work, the I just got blank browser page.
what is your plan for the next step? I will suggest you keep working on it, since I need this solution so I can use redux in rescript. Or you could create your own repo for this migration.

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.

ReasonReact to ReScript/React migration
3 participants