Repository navigation
TypeError: b is not a function #232
Description
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:
- Wonka is added to bsconfig.json with reason-urql, thus it gets compiled by ReScript and loaded directly as compiled ReScript dependency.
- Wonka is also loaded by
urqlwhich is ignored by ReScript and that import will load./dist/wonka.mjsusing import.
Activity
parkerziegler commentedon Dec 10, 2020
👋 Thanks for opening @Kingdutch!
Are you using
wonkav5with 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 onwonkav5 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
toSuspenseSourceinurql: https://github.com/FormidableLabs/urql/blob/685ca1828018da873d25d1b8bd27e18e3c8b5657/packages/react-urql/src/hooks/useQuery.ts#L78 In this scenario,urqlwould be settingsignal.tag, but ReScript's compilation ofwonkaexpectssignal.TAG. If that's indeed the case, there's not much we can do untilurqlbecomes compatible w/ v5 ofwonka(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 wonkaandyarn list --pattern wonkahere? Those should help us uncover any duplicate versions on disk in yournode_modules. Cheers!Kingdutch commentedon Dec 10, 2020
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:
Looks like there's only one actual version of
Wonkaon disk, although I see the ReScript compiler build all the.refiles and referens the.bs.jsfiles, while urql itself directly loads thedist/wonka.mjsfile 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
.tagvs.TAG. To get around this, I manually added asignal.TAG = 1line beneath it. This prevents the first error we sees but brings us to a new one.(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_ref3function (before therefdestructuring). This provides the following insights (I still have aconsole.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/catchthat logs a stacktrace and therefvalue. Whenever an error was thrown this was on the next line from that stacktrace since that's whererefwas 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
_ref3with the GraphQL query result.With the following trace.
3.
Without going through my component the
_reffunction is then called again but withundefinedas value. However this does not throw an error yet.The stacktrace from my component is as follows
4.
React now re-renders my component (I suspect this is the Development mode behaviour). Bringing us into the
_ref3function again. However, this time having theundefinedvalue and leading to the errorUncaught TypeError: ref is undefined.The stacktrace for this scenario is as follows
5.
The component is now actually rerendered again and reaches `_ref3` with an undefined value without logging an additional error.
6.
Then the component is rendered one final time reaching `_ref3` with `undefined` but again causing the `Uncaught TypeError: ref is undefined` error.
7.
Now finally the `react_devtools_backend.js` takes over and shows me an error message, stopping the execution of the React application.
8.
There is another
Uncaught TypeError: ref is undefinedbeneath the message in7which may suggest that5was not without error but the logging is slightly out of sync at that point.Kingdutch commentedon Dec 10, 2020
I've rebuilt React in
productionmode to see if this was indeed development mode behaviour. However, this shows the following in the terminal.Initial render and suspend
(data fetching happens here....)
With resolved data (still renders twice?):
The following two errors come right after the previous rendering logs.
The first in
react-dom.production.min.js:224:248The second from
urql.es.js:102:14.Kingdutch commentedon Dec 10, 2020
Found a solution!! :D
So adding the
signal.TAG = 1wasn't enough. Since it also no longer accesses asx.0but now usesx._0I also had to remapsignal._0 = signal[0];inurql.es.jsI'll create a PR with a change to the repository. I think it's safe to manually add that in a minor version of
urqlbecause 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 commentedon Dec 10, 2020
Hiya, just following this up from: urql-graphql/urql#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@5onurqland all of its packages on purpose (including an upgrade ofwonkaitself to ReScript). The information I was missing in the other PR is that a Yarn resolution was used to forcewonka@5to be used, so it's important to create discussions and issues before making any changes, since the solution would also be to make sure thattoSuspenseSourcewould 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/corelibrary 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
wonkawas compiled by it and some libraries would be using the.bs.jsfiles 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.Create simple reproduction
Kingdutch commentedon Dec 10, 2020
Right, yes, this was recommended by the documentation for
reason-urql.Sorry about that ^^ Do you still want me to open an issue in the
urqlrepository? I was already starting on that but can refrain from doing so if it doesn't help.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).Right, although
reason-urqlcurrently recommends the upgrade using the resolution (more on this in the reproduction description)Right, this is what I mentioned at the start of this issue, where
reason-urqlis used through ReScript by a ReScript application and useswonka/src/Client.bs.jswhile theurqlpackage itself (whose imports resolves towonka/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.mjscontains the following snippet using the new variant encoding (the snippet is an example, the new encoding is used throughout).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
urqlandwonka@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
urqlthat'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 byreason-urql(though it doesn't includereason-urqlitself).In the version without resolution entry the error doesn't occur because
urqlhas its ownnode_modules/wonkainstallation 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
urqlto 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-urqlto work with suspense (using its recommendations of aresolutionentry) while not changing the behaviour for people usingurqlwith 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.0andwonka@5.0.0-rc.1were mistakenly done with a BuckleScript/ReScript compiler >= 8 and thus releasing a versionwonka@5.0.0-rc.2that 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 commentedon Dec 10, 2020
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:
toSuspenseSourceimplementation. 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.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 commentedon Dec 10, 2020
Just for my own understanding I wanted to figure out how everything tied together.
The upgrade to BuckleScript 8 for Wonka happens in 0no-co/wonka#88 and notes
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
urqlmonorepo has been highlighted before. That was implemented in urql-graphql/urql@b23cdeaThe 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 commentedon Dec 10, 2020
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
pnpmode tries to solve. I'm not sure what problems occur if you force the community to use that though.The only real offence I see is that it tries to manually alter an objects' variant type instead of letting Wonka do this. If
urqloutsourced 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 commentedon Dec 10, 2020
Not quite; so basically if you have two packages
x -> wonka@^4.0.0andy -> wonka@^4.1.0let's say, then you'd expect that to resolve tox, y, wonka@^4.1.0, since they're both in range. However,yarnspecifically (maybe sometimesnpm? 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 theyarn-deduplicateCLI exists to fix this in the lockfile quickly. That's becauseyarn dedupedoesn't exist anymore (whilenpm dedupedoes). 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.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.
Like I said, yea, we had several versions of this which all didn't use internals, but testing new versions of
toSuspenseSourceis an arduous process.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
urqlin general gettingTypeError: xyz is not a functionin random places on upgrading for instance.Kingdutch commentedon Dec 10, 2020
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 commentedon Dec 11, 2020
@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 inreason-urql, other than encouraging users to avoidsuspenseuntil we can migrate towonkav5 upstream. I'll keep this issue updated as we work on the migration over inurql, 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 commentedon Dec 11, 2020
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
urqlitself feel free to tag me there or ping me on Discord :D