Skip to content

Revert "Fix Js.String.match_ return type" because it's a breaking change with ReScript #319

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
wants to merge 2 commits into from

Conversation

jfrolich
Copy link
Contributor

I think it's a good change in general, but I think we should be careful about breaking with ReScript, as it makes parallel use of both compilers impossible. Which is something companies/projects want to do to support both compilers when experimenting with melange. I would suggest when making these changes to the bindings to create a new function instead of changing the old one breaking compatibility.

This reverts commit ccceb69.

@jfrolich jfrolich closed this Jun 12, 2022
@jfrolich jfrolich force-pushed the revert-breaking-change branch from 359d9ca to f5cb19b Compare June 12, 2022 09:29
@jfrolich jfrolich reopened this Jun 12, 2022
@anmonteiro
Copy link
Member

Why is this breaking ReScript compatibility? I cherry picked the commit from rescript-lang/rescript#5070

@jchavarri
Copy link
Member

I think what @jfrolich means is that it might become hard to track which upstream breaking changes have made it into Melange. This change in particular did not make it until ReScript v10: rescript-lang/rescript-lang.org#552.

I suggest that we start versioning Melange (maybe also publishing in opam), so that we can map Melange versions with ReScript versions somehow, e.g.

Melange ReScript
v0.1.x v9.x
v0.2.x v10.x
v0.3.x v11.x
... ...

That way, Melange users have an easier way of tracking which breaking changes made it to the codebase, and which ReScript libraries might be supported for each version of Melange.

If the above makes sense, then we could:

  • merge this PR
  • publish v0.1
  • revert PR
  • include any other changes in ReScript v10
  • start publishing v0.2 versions
  • ...

@anmonteiro what do you think?

@jfrolich
Copy link
Contributor Author

Exactly! For this particular change it's water under the bridge (we will be at rescript 10 soon). But for future breaking changes having this system would be amazing!

@anmonteiro
Copy link
Member

Makes sense to do that. I'll try keeping those notes next time I batch a few cherry picked commits.

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.

3 participants