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

Add compatibility for Wonka compiled with ReScript >= 8 #1214

Closed
wants to merge 1 commit into from
Closed

Add compatibility for Wonka compiled with ReScript >= 8 #1214

wants to merge 1 commit into from

Conversation

Kingdutch
Copy link
Contributor

Summary

It took a bit of investigating but it turns out that when Wonka is compiled with ReScript >= 8 it's variant representation changes. Urql has one location where it manually encodes a variant in userQuery, thus this cause problems when Urql expects Wonka to be compiled with ReScript < 8, but that's not the case.

The full investigation story can be found in teamwalnut/rescript-urql#232

I searched for the usage of .tag in the codebase to find where this manual encoding was performed and only found this in react-urql and preact-urql.

A better change than my fix would be to let Wonka encode the variant (perhaps we can expose a function?) so that it automatically uses whatever encoding ReScript decided. This more properly decouples Urql from ReScript's internal decisions and avoids breakage in the future if they decide to further change this representation.

Set of changes

Urql's useQuery for React and Preact now encodes it's cache results in a manner compatible with ReScript >= 8 as well as ReScript < 8 to ensure that Urql doesn't break when Wonka has been compiled with newer ReScript versions.

@changeset-bot
Copy link

changeset-bot bot commented Dec 10, 2020

⚠️ No Changeset found

Latest commit: 48ed22b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

ReScript has new variant encoding from version 8 onwards.
This means that if Wonka is compiled with ReScript 8 or newer then our
manual encoding of a variant passed to `sink` doesn't work. To remedy
this we add the new variant encoding next to the old one. ReScript will
ignore the encoding it doesn't need so this should with versions of
ReScript before and after the change.
@kitten
Copy link
Member

kitten commented Dec 10, 2020

First of all; I do have to politely ask you to read the CONTRIBUTING.md guide for PRs and documenting changes. Furthermore, we may need an issue for this.

I was aware that this changes some internals, but using the new version of Wonka is deeply unsettling to me, since, compiled with BS 8 / RS, it isn't compatible with any other version of it. (Or using the old version with the newer compilation; same same, but worse)

Now BuckleScript has always been making "curious choices" in terms of transpilation and not using bundles / packages properly which it really should.

This wouldn't be an issue, but were sometimes already encountering people that have duplicate installations of @urql/core.
This is less severe since this will most of the times only cause type issues.

With Wonka it could be the same, except that the two different ways of compiling these types are inherently incompatible with one another. Which is suboptimal given that different versions of exchanges could be installed.

So this does fix it, but opens up a new problem that we didn't want to address just yet.

I'll close it for now, since it's unlikely to completely solve this whole problem, but I'm aware that it would appear to fix it temporarily.

That being said, the simplest thing to do on your end is not to use suspense-mode if at all possible with reason-urql

@kitten
Copy link
Member

kitten commented Dec 10, 2020

More info here: teamwalnut/rescript-urql#232 (comment) So an additional issue in this repo may not be needed yet, since we haven't even scheduled a wonka upgrade or changes thereof as part of our RFC pipeline.

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.

2 participants