search

teamwalnut / rescript-urql

ReScript bindings for Formidable's Universal React Query Library, urql.
MIT License
238 stars 28 forks source link

TypeError: b is not a function #232

Open Kingdutch opened 3 years ago

Kingdutch commented 3 years ago

Using React suspense (0.0.0-experimental-d7382b6c4) with reason-urql I run into the following error.

Uncaught TypeError: b is not a function
    d webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:142
    1 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:814
    toSuspenseSource webpack://social_chat/./node_modules/urql/dist/urql.es.js?:197
    1 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:801
    x webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:155
    _0 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:76
    _0 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:166
    d webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:290
    x webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:158
    I webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:303
    d webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:141
    1 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:814
    _0 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:710
    d webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:142
    1 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:814
    1 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:703
    1 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:801
    x webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:155
    _0 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:76
    _0 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:166
    _0 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:314
    L webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:347
    1 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:947
    I webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:306
    1 webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:934
    L webpack://social_chat/./node_modules/wonka/dist/wonka.mjs?:346
    1 webpack://social_chat/./node_modules/urql/dist/urql.es.js?:113
    React 3
    useSource webpack://social_chat/./node_modules/urql/dist/urql.es.js?:108
    useQuery webpack://social_chat/./node_modules/urql/dist/urql.es.js?:213
    useQuery webpack://social_chat/./node_modules/reason-urql/src/hooks/UseQuery.bs.js?:84
    Overview webpack://social_chat/./src/screens/Overview.bs.js?:57
   [...]

A console.log before the useQuery hook invocation seems to indicate this happens after the rerender caused by React development mode (the network request completes but contains the following error.

{
  "data": { "currentUser" :null },
  "extensions" :[{
    "message":"first may not be larger than 100",
    "locations":[{"line":3,"column":5}],
    "path" :["currentUser","conversations"]
  } ]
}

This is produced by the query 

module GetConversations = %graphql(`
  query user_conversations($first: Int!) {
    currentUser {
       conversations(first: $first) {
         nodes {
           id
           name
         }
       }
    }
  }
`)

Phil (Kitten) suggested that it could be caused by multiple instances of Wonka. I'm not sure how I'd debug that. However, I could come up with a possibility for that which I would not know how to resolve:

parkerziegler commented 3 years ago

👋 Thanks for opening @Kingdutch!

Are you using wonka v5 with yarn resolutions? This sounds suspiciously similar to #206, but the opacity of the stack trace with the MJS build makes it harder to confirm whether or not it's the same bug. If you aren't on wonka v5 my guess is its the same issue, which is caused by BuckleScript / ReScript changing some internals across majors. You can read here for more details. The fix there is to ensure you're using yarn resolutions and resolving v5, which should force a single version to be resolved.

If it's still not working and you've confirmed the above, then my next guess, based on the stack trace, is that it has to do with this line inn toSuspenseSource in urql: https://github.com/FormidableLabs/urql/blob/685ca1828018da873d25d1b8bd27e18e3c8b5657/packages/react-urql/src/hooks/useQuery.ts#L78 In this scenario, urql would be setting signal.tag, but ReScript's compilation of wonka expects signal.TAG. If that's indeed the case, there's not much we can do until urql becomes compatible w/ v5 of wonka (likely destined for a future major). A bit of an odd case where ReScript's decision to change internals bites us across compiler versions.

Lastly, can you provide the output of running yarn why wonka and yarn list --pattern wonka here? Those should help us uncover any duplicate versions on disk in your node_modules. Cheers!

Kingdutch commented 3 years ago

Are you using wonka v5 with yarn resolutions?

Yes! ^^ Made sure to follow the docs before posting here. This is in my yarn.lock file, indicating it resolved to 5.0.0-rc.1:

wonka@5.0.0-rc.1, wonka@^4.0.14:
  version "5.0.0-rc.1"
  resolved: <snip>
  integrity: <snip>
$ yarn why wonka
yarn why v1.22.10
[1/4] 🤔  Why do we have the module "wonka"...?
[2/4] 🚚  Initialising dependency graph...
warning Resolution field "wonka@5.0.0-rc.1" is incompatible with requested version "wonka@^4.0.14"
warning Resolution field "wonka@5.0.0-rc.1" is incompatible with requested version "wonka@^4.0.14"
warning Resolution field "wonka@5.0.0-rc.1" is incompatible with requested version "wonka@^4.0.14"
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "wonka@5.0.0-rc.1"
info Reasons this module exists
   - "@urql#exchange-graphcache" depends on it
   - Hoisted from "@urql#exchange-graphcache#wonka"
   - Hoisted from "urql#wonka"
   - Hoisted from "@urql#exchange-graphcache#@urql#core#wonka"
info Disk size without dependencies: "3.65MB"
info Disk size with unique dependencies: "3.65MB"
info Disk size with transitive dependencies: "3.65MB"
info Number of shared dependencies: 0
✨  Done in 0.91s.
$ yarn list --pattern wonka
yarn list v1.22.10
warning Resolution field "wonka@5.0.0-rc.1" is incompatible with requested version "wonka@^4.0.14"
warning Resolution field "wonka@5.0.0-rc.1" is incompatible with requested version "wonka@^4.0.14"
warning Resolution field "wonka@5.0.0-rc.1" is incompatible with requested version "wonka@^4.0.14"
└─ wonka@5.0.0-rc.1
✨  Done in 0.62s.

Looks like there's only one actual version of Wonka on disk, although I see the ReScript compiler build all the .re files and referens the .bs.js files, while urql itself directly loads the dist/wonka.mjs file that comes from NPM. So there could still be 2 versions of Wonka (one pre-compiled, one built with the project) in the bundled output.

I was able to clean up the stack trace by changing the source map generation in Webpack (didn't think of that last night). This confirmed your suspicion on the problem being caused by .tag vs .TAG. To get around this, I manually added a signal.TAG = 1 line beneath it. This prevents the first error we sees but brings us to a new one.

Uncaught TypeError: ref is undefined
    _ref3 urql.es.js:103
    1 wonka.mjs:757
    toSuspenseSource urql.es.js:160

(those lines actually match the files)

To debug this further I added try { throw new Error() } catch (err) { console.log(ref, err); } to the top of the _ref3 function (before the ref destructuring). This provides the following insights (I still have a console.log("rendering") before the hook invocation in my element to see what React does to it, these are not shown).

Below contains the events as I see them in the console, along with the stacktraces from my own try/catch that logs a stacktrace and the ref value. Whenever an error was thrown this was on the next line from that stacktrace since that's where ref was accessed. I've used headings because the details block didn't work nicely with an ordered list ^^'

1.

On the first component render the hook is called and we suspend.

2.

The data is successfully fetched and the component is rerendered. This calls the hook and leads to call _ref3 with the GraphQL query result.

With the following trace. ``` _ref3 urql.es.js:102 1 wonka.mjs:757 1 wonka.mjs:1087 b wonka.mjs:910 b wonka.mjs:909 1 wonka.mjs:794 1 wonka.mjs:815 1 wonka.mjs:1064 1 wonka.mjs:511 b wonka.mjs:910 b wonka.mjs:909 F wonka.mjs:209 d wonka.mjs:157 1 wonka.mjs:763 1 wonka.mjs:511 b wonka.mjs:910 b wonka.mjs:909 1 wonka.mjs:511 next wonka.mjs:735 next wonka.mjs:734 dispatchOperation urql-core.mjs:382 node_modules main.js:2271 node_modules main.js:2299 1 wonka.mjs:815 1 wonka.mjs:1066 1 wonka.mjs:814 1 wonka.mjs:784 1 wonka.mjs:922 1 wonka.mjs:1083 toSuspenseSource urql.es.js:156 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 d wonka.mjs:239 x wonka.mjs:107 I wonka.mjs:252 d wonka.mjs:90 1 wonka.mjs:763 _0 wonka.mjs:659 d wonka.mjs:91 1 wonka.mjs:763 1 wonka.mjs:652 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 _0 wonka.mjs:263 L wonka.mjs:296 1 wonka.mjs:896 I wonka.mjs:255 1 wonka.mjs:883 L wonka.mjs:295 1 urql.es.js:74 React 3 useSource urql.es.js:69 useQuery urql.es.js:176 useQuery UseQuery.bs.js:74 Overview Overview.bs.js:52 React 8 workLoop scheduler.development.js:597 flushWork scheduler.development.js:552 performWorkUntilDeadline scheduler.development.js:164 js scheduler.development.js:187 js scheduler.development.js:857 Webpack 16 ```

3.

Without going through my component the _ref function is then called again but with undefined as value. However this does not throw an error yet.

The stacktrace from my component is as follows ``` _ref3 urql.es.js:102 1 wonka.mjs:757 toSuspenseSource urql.es.js:160 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 d wonka.mjs:239 x wonka.mjs:107 I wonka.mjs:252 d wonka.mjs:90 1 wonka.mjs:763 _0 wonka.mjs:659 d wonka.mjs:91 1 wonka.mjs:763 1 wonka.mjs:652 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 _0 wonka.mjs:263 L wonka.mjs:296 1 wonka.mjs:896 I wonka.mjs:255 1 wonka.mjs:883 L wonka.mjs:295 1 urql.es.js:74 React 3 useSource urql.es.js:69 useQuery urql.es.js:176 useQuery UseQuery.bs.js:74 Overview Overview.bs.js:52 React 8 workLoop scheduler.development.js:597 flushWork scheduler.development.js:552 performWorkUntilDeadline scheduler.development.js:164 js scheduler.development.js:187 js scheduler.development.js:857 Webpack 16 ```

4.

React now re-renders my component (I suspect this is the Development mode behaviour). Bringing us into the _ref3 function again. However, this time having the undefined value and leading to the error Uncaught TypeError: ref is undefined.

The stacktrace for this scenario is as follows ``` _ref3 urql.es.js:102 1 wonka.mjs:757 toSuspenseSource urql.es.js:160 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 d wonka.mjs:239 x wonka.mjs:107 I wonka.mjs:252 d wonka.mjs:90 1 wonka.mjs:763 _0 wonka.mjs:659 d wonka.mjs:91 1 wonka.mjs:763 1 wonka.mjs:652 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 _0 wonka.mjs:263 L wonka.mjs:296 1 wonka.mjs:896 I wonka.mjs:255 1 wonka.mjs:883 L wonka.mjs:295 1 urql.es.js:74 React 3 useSource urql.es.js:69 useQuery urql.es.js:176 useQuery UseQuery.bs.js:74 Overview Overview.bs.js:52 React 11 workLoop scheduler.development.js:597 flushWork scheduler.development.js:552 performWorkUntilDeadline scheduler.development.js:164 js scheduler.development.js:187 js scheduler.development.js:857 Webpack 16 ```

5.

The component is now actually rerendered again and reaches `_ref3` with an undefined value without logging an additional error. ``` _ref3 urql.es.js:102 1 wonka.mjs:757 toSuspenseSource urql.es.js:160 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 d wonka.mjs:239 x wonka.mjs:107 I wonka.mjs:252 d wonka.mjs:90 1 wonka.mjs:763 _0 wonka.mjs:659 d wonka.mjs:91 1 wonka.mjs:763 1 wonka.mjs:652 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 _0 wonka.mjs:263 L wonka.mjs:296 1 wonka.mjs:896 I wonka.mjs:255 1 wonka.mjs:883 L wonka.mjs:295 1 urql.es.js:74 React 3 useSource urql.es.js:69 useQuery urql.es.js:176 useQuery UseQuery.bs.js:74 Overview Overview.bs.js:52 React 8 workLoop scheduler.development.js:597 flushWork scheduler.development.js:552 performWorkUntilDeadline scheduler.development.js:164 js scheduler.development.js:187 js scheduler.development.js:857 Webpack 16 ```

6.

Then the component is rendered one final time reaching `_ref3` with `undefined` but again causing the `Uncaught TypeError: ref is undefined` error. ``` _ref3 urql.es.js:102 1 wonka.mjs:757 toSuspenseSource urql.es.js:160 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 d wonka.mjs:239 x wonka.mjs:107 I wonka.mjs:252 d wonka.mjs:90 1 wonka.mjs:763 _0 wonka.mjs:659 d wonka.mjs:91 1 wonka.mjs:763 1 wonka.mjs:652 1 wonka.mjs:750 x wonka.mjs:104 _0 wonka.mjs:25 _0 wonka.mjs:115 _0 wonka.mjs:263 L wonka.mjs:296 1 wonka.mjs:896 I wonka.mjs:255 1 wonka.mjs:883 L wonka.mjs:295 1 urql.es.js:74 React 3 useSource urql.es.js:69 useQuery urql.es.js:176 useQuery UseQuery.bs.js:74 Overview Overview.bs.js:52 React 11 workLoop scheduler.development.js:597 flushWork scheduler.development.js:552 performWorkUntilDeadline scheduler.development.js:164 js scheduler.development.js:187 js scheduler.development.js:857 ```

7.

Now finally the `react_devtools_backend.js` takes over and shows me an error message, stopping the execution of the React application. ``` The above error occurred in the component: in Overview (created by ChatWindow) in Suspense (created by ChatWindow) in ChatWindow (created by App) in div (created by ForwardRef(MotionComponent)) in ForwardRef(MotionComponent) (created by OpenSocial__Components__Molecules__Tray) in OpenSocial__Components__Molecules__Tray (created by App) in PresenceChild (created by AnimatePresence) in AnimatePresence (created by App) in IntlProvider (created by App) in App Consider adding an error boundary to your tree to customize error handling behavior. Visit https://fb.me/react-error-boundaries to learn more about error boundaries. react_devtools_backend.js:2430:23 ```

8.

There is another Uncaught TypeError: ref is undefined beneath the message in 7 which may suggest that 5 was not without error but the logging is slightly out of sync at that point.

Kingdutch commented 3 years ago

I've rebuilt React in production mode to see if this was indeed development mode behaviour. However, this shows the following in the terminal.

Initial render and suspend

rendering Overview.bs.js:51:10

(data fetching happens here....)

With resolved data (still renders twice?):

rendering 2 Overview.bs.js:51:10

The following two errors come right after the previous rendering logs.

The first in react-dom.production.min.js:224:248

TypeError: e is undefined
    Ot urql.es.js:102
    T wonka.mjs:755
    a urql.es.js:159
    T wonka.mjs:748
    y wonka.mjs:102
    _0 wonka.mjs:23
    _0 wonka.mjs:113
    i wonka.mjs:237
    y wonka.mjs:105
    h wonka.mjs:250
    i wonka.mjs:88
    T wonka.mjs:761
    _0 wonka.mjs:657
    i wonka.mjs:89
    T wonka.mjs:761
    b wonka.mjs:650
    T wonka.mjs:748
    y wonka.mjs:102
    _0 wonka.mjs:23
    _0 wonka.mjs:113
    _0 wonka.mjs:261
    m wonka.mjs:294
    Pt wonka.mjs:894
    h wonka.mjs:253
    Pt wonka.mjs:881
    m wonka.mjs:293
    l urql.es.js:74
    React 2
    u urql.es.js:69
    It urql.es.js:175
    o UseQuery.bs.js:57
    mr Overview.bs.js:52
    React 6
    V scheduler.production.min.js:17
    onmessage scheduler.production.min.js:14
    53 scheduler.production.min.js:13
    Webpack 10

The second from urql.es.js:102:14.


Uncaught TypeError: e is undefined
    Ot urql.es.js:102
    T wonka.mjs:755
    a urql.es.js:159
    T wonka.mjs:748
    y wonka.mjs:102
    _0 wonka.mjs:23
    _0 wonka.mjs:113
    i wonka.mjs:237
    y wonka.mjs:105
    h wonka.mjs:250
    i wonka.mjs:88
    T wonka.mjs:761
    _0 wonka.mjs:657
    i wonka.mjs:89
    T wonka.mjs:761
    b wonka.mjs:650
    T wonka.mjs:748
    y wonka.mjs:102
    _0 wonka.mjs:23
    _0 wonka.mjs:113
    _0 wonka.mjs:261
    m wonka.mjs:294
    Pt wonka.mjs:894
    h wonka.mjs:253
    Pt wonka.mjs:881
    m wonka.mjs:293
    l urql.es.js:74
    React 2
    u urql.es.js:69
    It urql.es.js:175
    o UseQuery.bs.js:57
    mr Overview.bs.js:52
    React 6
    V scheduler.production.min.js:17
    onmessage scheduler.production.min.js:14
    53 scheduler.production.min.js:13
    Webpack 10
Kingdutch commented 3 years ago

Found a solution!! :D

So adding the signal.TAG = 1 wasn't enough. Since it also no longer accesses as x.0 but now uses x._0 I also had to remap signal._0 = signal[0]; in urql.es.js

I'll create a PR with a change to the repository. I think it's safe to manually add that in a minor version of urql because it creates forward compatibility with the newer BuckleScript compiler but is ignored by older BuckleScript compiler, from what I can tell (hopefully the test suite lets us know if that's not the case).

kitten commented 3 years ago

Hiya, just following this up from: https://github.com/FormidableLabs/urql/pull/1214

This isn't quite as simple yet, and I'd love to find a solution for this that will keep this running but we've actually explicitly deferred the upgrade of wonka@5 on urql and all of its packages on purpose (including an upgrade of wonka itself to ReScript). The information I was missing in the other PR is that a Yarn resolution was used to force wonka@5 to be used, so it's important to create discussions and issues before making any changes, since the solution would also be to make sure that toSuspenseSource would be migrated to a proper source, since it's current form its implementation is optimised for size and reduced on purpose, as it (in a stable form) wouldn't drop into the data structure internals.

There are some questions I'll need to look into before we can make any changes. The main problem is that once we'd upgrade Wonka we'd have to make sure that it's impossible to have two versions of Wonka streams in one system, e.g. by having outdated exchanges. Typically the @urql/core library can always be deduplicated since its range is quite freely set and it's unimpactful for exchanges, but for Wonka this isn't the same, as the underlying structures would be incompatible and exchanges are using this explicitly. This would negatively impact all users; especially if they aren't using TypeScript, which would mean that they wouldn't even get a type error on mismatching streams.

Also, there were issues with BuckleScript@<=8 before that meant that wonka was compiled by it and some libraries would be using the .bs.js files and some the bundle. This would work but lead to duplication, which also isn't optimal and could be making this issue much worse (since then we'd have up to three versions of the same implementation). So this is something I have to look into in respect to a ReScript upgrade.

Kingdutch commented 3 years ago

The information I was missing in the other PR is that a Yarn resolution was used to force wonka@5 to be used

Right, yes, this was recommended by the documentation for reason-urql.

so it's important to create discussions and issues before making any changes

Sorry about that ^^ Do you still want me to open an issue in the urql repository? I was already starting on that but can refrain from doing so if it doesn't help.

since the solution would also be to make sure that toSuspenseSource would be migrated to a proper source, since it's current form its implementation is optimised for size and reduced on purpose, as it (in a stable form) wouldn't drop into the data structure internals.

I see, yeah my PR noted that ideally it doesn't handle the internals at all but defers to Wonka. Since this is only used in 1 case, would an asVariant (or similar) call add size? I guess I can see the slippery slope "but it's only a few characters we're adding here" (unless I misunderstand the impact).

The main problem is that once we'd upgrade Wonka we'd have to make sure that it's impossible to have two versions of Wonka streams in one system, e.g. by having outdated exchanges.

Right, although reason-urql currently recommends the upgrade using the resolution (more on this in the reproduction description)

Also, there were issues with BuckleScript@<=8 before that meant that wonka was compiled by it and some libraries would be using the .bs.js files and some the bundle.

Right, this is what I mentioned at the start of this issue, where reason-urql is used through ReScript by a ReScript application and uses wonka/src/Client.bs.js while the urql package itself (whose imports resolves to wonka/dist/wonka.mjs). However, I believe that's not the problem here.

The main cause of the issue here seems to be that Wonka (>=5), as distributed on npm, has been compiled with BuckleScript/ReScript >= 8.0 since wonka/dist/wonka.mjs contains the following snippet using the new variant encoding (the snippet is an example, the new encoding is used throughout).

function map$1(a) {
  return function (b) {
    return function (c) {
      return b(function (b) {
        b =
          'number' == typeof b
            ? 0
            : b.TAG
            ? {
                TAG: 1,
                _0: a(b._0),
              }
            : {
                TAG: 0,
                _0: b._0,
              };
        c(b);
      });
    };
  };
}

This also means that the incompatibility is not dependent on whatever version of BuckleScript/ReScript my project is using, but this is an incompatibility between urql and wonka@5.0.0-rc.1. It appears the wonka@5 versions are built with ReScript >= 8, which can be seen in the wonka.js and wonka.mjs published to npm.

The change I proposed in the PR would add the variant encoding for the single usage in urql that's compatible with ReScript >= 8 compilations of Wonka. This ensures it works with Wonka 4 and 5.

Reduced reproduction

I've created a repository with a simple test-case that doesn't contain reason-urql (nor ReScript/BuckleScript at all) to demonstrate the problem. The repository installs wonka@5.0.0-rc.1 which is needed by reason-urql (though it doesn't include reason-urql itself).

In the version without resolution entry the error doesn't occur because urql has its own node_modules/wonka installation and there are two versions of Wonka.

In the version with resolution entry there is only one version of Wonka installed (as visible in the diff). This causes suspense with urql to fail because the Variant representation in Wonka 5 is different than in Wonka 4.

You can see the error for yourself by cloning the repository to the desired commit and running yarn install && yarn start.

What my PR tried to change

The PR I proposed (I've adjusted it to include a changeset and made sure all the checks passed locally, sorry for not reading the contributors guide beforehand), added the fields for variant resolution as used in ReScript >= 8 (and as a result Wonka 5) while leaving the fields for variant resolution in ReScript < 8 intact. This allows reason-urql to work with suspense (using its recommendations of a resolution entry) while not changing the behaviour for people using urql with Wonka 4.

An alternative

As you mention that the update for Wonka to ReScript has been purposefully held up, then it appears that the compilation of wonka@5.0.0-rc.0 and wonka@5.0.0-rc.1 were mistakenly done with a BuckleScript/ReScript compiler >= 8 and thus releasing a version wonka@5.0.0-rc.2 that was compiled with BuckleScript/ReScript < 8 would also solve the problem (by reverting Wonka's variant representation to the old format).

The blogpost that described the variant representation change can be found in the ReScript blog archives: https://rescript-lang.org/blog/overview-of-new_encoding#variant-internal

kitten commented 3 years ago

Yep, I'm aware of all of the changes and the compilation differences (which is why v5 exists in the first place)

What I'm more concerned about us that we have to switch all of urql over to the new compilation sooner or later. What that means is that people may upgrade some package and get incompatible exchanges created using different versions of Wonka.

I have come up with a migration plan though which may work:

  1. Replace the toSuspenseSource implementation. Believe it or not, we had like five variants of it but stayed with the "hacky" but proven one you saw because Suspense can only be tested manually by hand against some sandboxes and builds. That's why we didn't test alternatives yet as much when we made changes.
  2. Wonka's compilation change should occur in a minor version, not a major, to increase the chance of people upgrading it landing on the same one once they deduplicate packages (package managers like to mess with this, so there will still be mismatches)
  3. `@urql/core needs to ensure that all versions of exchanges are running the same version of Wonka. This does leave out the bindings, but most people are upgrading core and bindings in lockstep or are using core only via the bindings. (And as long as we tell them to deduplicate this may be fine)

I have a branch which does all of this (minus of course changes to the version of Wonka) but this will need some careful planning on our part 😅🤔 https://github.com/FormidableLabs/urql/tree/fix/pure-tosuspensesource

Kingdutch commented 3 years ago

Just for my own understanding I wanted to figure out how everything tied together.

The upgrade to BuckleScript 8 for Wonka happens in https://github.com/kitten/wonka/pull/88 and notes

Since it will be a break change for TS/JS user, I suggest to release it as wona@5-alpha.0 So this is indeed not an accidental upgrade but a concious breakage.

BuckleScript's variant implementation is directly exposed to TypeScript through https://github.com/kitten/wonka/blob/master/src/shims/Js.shim.ts (and a flow file is also generated) which means that any implementation has to work with the fields in the records directly to figure out the state of a signal.

The only instance of this that I can find in the urql monorepo has been highlighted before. That was implemented in https://github.com/FormidableLabs/urql/pull/1123/commits/b23cdea41bb0a62e8523aabb37c6726053e5ab04

The result of this exposure is the bug that started this issue. It also means that if BuckleScript decides at any point to change its variant representation again (for performance/space saving/whatever reasons), this will require a major version of Wonka that ripples through its ecosystem.

My knowledge of TypeScript is too limited to find a solution, but I'm wondering if there's a way to encode BuckleScript's variant representation in TypeScript so we can prevent such breaking changes in the future (and only need it once).

Kingdutch commented 3 years ago

Yep, I'm aware of all of the changes and the compilation differences (which is why v5 exists in the first place)

Ha! Yeah as you can see above, I was tracing out the history here to figure this out myself. If anyone else comes to this and they want to know the course of events, now they know too :D

The multiple copies issue is something that yarn's pnp mode tries to solve. I'm not sure what problems occur if you force the community to use that though.

Replace the toSuspenseSource implementation. Believe it or not, we had like five variants of it but stayed with the "hacky" but proven one you saw because Suspense can only be tested manually by hand against some sandboxes and builds. That's why we didn't test alternatives yet as much when we made changes.

The only real offence I see is that it tries to manually alter an objects' variant type instead of letting Wonka do this. If urql outsourced this then it would no longer be dependent on the internal representation. Is it common in exchanges that they manipulate these fields like that?

What's the best way to see the other breaking changes in Wonka moving from 4 -> 5?

I guess without having written my own exchange this is about the limit of my understanding ^^'

EDIT: Took a look at the branch you linked.

I like the approach! That at least should make it clearer to users what's going on and prevent subtle bugs due to incompatibility once a new release of urql is made! :D

It'd be interesting to see if we can create a codemod to help upgrade any exchanges that may be using the internals directly. Alternatively, if it's determined this is bad practice altogether and shouldn't be done. Perhaps a development mode check or linting rule can identify that it's happening and warn that it makes upgrading more difficult.

kitten commented 3 years ago

Not quite; so basically if you have two packages x -> wonka@^4.0.0 and y -> wonka@^4.1.0 let's say, then you'd expect that to resolve to x, y, wonka@^4.1.0, since they're both in range. However, yarn specifically (maybe sometimes npm? Haven't seen that to be fair, so probably not) does something odd when you upgrade or add new packages and may sometimes do: x, x/node_modules/wonka@^4.0.0, y, y/node_modules/wonka@^4.1.0 (rough principle). This is so common of an issue that the yarn-deduplicate CLI exists to fix this in the lockfile quickly. That's because yarn dedupe doesn't exist anymore (while npm dedupe does). This ceased to exist because these version overlaps should always be resolveable. There are some exceptions of course, but we rely on this because these exceptions don't apply for us. So this may be fixed in yarn pnp, but just because it's fixed there doesn't mean it's an impossible state.

I'm not sure what problems occur if you force the community to use that though.

Precisely, that's the other issue. I'm also not convinced that's the solution, because we're not aiming to support a situation where one package manager is more preferable to another in all cases.

The only real offence I see is that it tries to manually alter an objects' variant type instead of letting Wonka do this. That at least should make it clearer to users what's going on and prevent subtle bugs

Like I said, yea, we had several versions of this which all didn't use internals, but testing new versions of toSuspenseSource is an arduous process.

It'd be interesting to see if we can create a codemod to help upgrade any exchanges that may be using the internals directly.

None do; even custom ones I wouldn't expect to since it's never necessary. So I don't really mind breakage if some do, because at that point breakage is expected.

The main issue here is just addressing an upgrade across our numerous exchange packages. So it's not a technical or code problem per se, it's a problem of convenience for users and of the process of doing so safely, since if this causes confusing issues, it's intransparent; image everyone who uses urql in general getting TypeError: xyz is not a function in random places on upgrading for instance.

Kingdutch commented 3 years ago

Right, so if I understand correctly. Once urql's current use of Wonka's internals is removed then that should mean that no (well behaving/supported) exchanges should be using Wonka's internals. That means that (for this particular issue) upgrading to Wonka 5 is a non-breaking change from urql`s point of view. (I don't know if there are other breaking changes in Wonka?)

The only problem that's left is if exchanges' requirements cause a mixed set of wonka versions to be installed by a package manager. Passing data between those two instances would cause users a bad time. Your branch includes a check that detects this painful scenario. However, it assumes that wonka 4.1 is the only correct version.

If the only breaking change between wonka 4 and 5 is the change in internal representation. Then any well behaved exchange that doesn't touch the internal representation itself can already mark itself compatible with wonka 5 (i.e. update dependencies to "wonka": "^4.1 || ^5.0").

That means your check could be relaxed to only validate that all exchanges use the same version of Wonka, without enforcing a specific one. This means all exchanges can mark their compatibility with the future version but users only actually have to switch once all are compatible.

(If there are other breaking changes between wonka 4 -> 5 then the above goes out the window)

parkerziegler commented 3 years ago

@Kingdutch I've attached the future label to this since it'll be coming along with a future release of urql. For now, I'm not sure there's much to be done here in reason-urql, other than encouraging users to avoid suspense until we can migrate to wonka v5 upstream. I'll keep this issue updated as we work on the migration over in urql, but I think this is likely a priority for the New Year. Thanks for opening and doing so much to help track down the root issue 👍

Kingdutch commented 3 years ago

You're most welcome. Thanks to Phil and yourself for the very rapid responses and for providing insights into the issues and roadmap! If there is anything I can help with to push this forward in urql itself feel free to tag me there or ping me on Discord :D