urql shouldn't be a dep but a peer dep

[MoOx]

Hi again,

I will make this short. I am still trying to use reason-urql with next! I am getting this issue:

As you can see, the code produced by reason-urql seems zero-cost at some point (I can tell when I read [@bs.module "urql"] external). But when you do such bindings, you need to be sure that the dep in question (urql here) is available at the correct level in node_modules. Here my ./pages/index.bs.js try (because of the code produced by rescript/bucklescript) to reach urql itself. And it's not available since it's specified as a dep in reason-urql.

For reason-react-native for example (which are only zero cost bindings), react-native is a peer dep & not a dep.

You should probably remove urql from deps & put it as peer dep + document this in the installation section of the readme.

[parkerziegler]

Hey @MoOx, I began looking at this but have had a recent death in my family and won't be getting back to it for a few days (or, more likely, a week and a half). My first impressions:

• As far as I know, urql should still be located at /node_modules/urql after installing reason-urql. I suppose I can't say for sure on npm, but a fresh installation using yarn does yield this result.

So I'm not sure that the location of urql in node_modules is the root problem here. In general, I believe package managers since npm@3 flatten all dependencies of dependencies, so whether urql is a peerDep or a dep it would still be located at /node_modules/urql.

• Are you doing anything in next.config.js that would alter the standard Node resolution algorithm? As far as I understand Node resolution, it'll start at the root and recursively move one directory up searching for node_modules, until it reaches globally installed dependencies. Do you have any resolve.alias or resolve.modules rules set up?

Whether or not urql should be a direct or peer dep is a complicated question because of urql's versioning. If urql wasn't a direct dependency it would be very difficult to ensure that the version of bindings a user was using (reason-urql) was compatible with their urql version. New urql versions tend to be released ahead of reason-urql, and we don't have sound guarantees that our bindings will work with any minor version.

@kitten I'd be curious about your thoughts here. We did have urql as a peerDependency in the past but opted to make it a direct dependency to support:

• Single package install
• Guaranteed APIs that work with our bindings based on locked versions

[kitten]

In this case we expect urql to be a dependency of reason-urql. In general all bindings have @urql/core as a dependency directly and exchanges do the same. This works because their ranges are always compatible, so a package manager will hoist them.

When it comes to urql once it's used directly it should be installed separately.

There are drawbacks with this in that duplicates are at times possible. With @urql/core this doesn't matter too much because the Client will only be instantiated in one place, hence the worst case scenario are some duplicate helper functions or types.

In the case of reason-urql a duplicate urql dependency is more severe. So I'd be on the side of making this a peer dependency.

However this discussion is separate from the issue and I'd prefer investigating that first.

The reason for that is that we shouldn't jump ship and go from an issue report immediately to a proposed solution. In this case that's important because regardless of whether urql is a peer dependency or not, it should always be resolved correctly. So that's unrelated 😅

[MoOx]

@parkerziegler My condolescences.

I totally get what you are explaining. In fact I already investigated all of this before opening this issue. Here is what I did:

• deleted node_modules
• deleted yarn.lock
• yarn (using yarn v1.x)
• checked node_modules for urql at the root: no luck it's not here. It's in node_modules/reason-urql/node_modules.
• I tried to understand why, as I know yarn do whatever it can to flatten: the only thing I found is that graphql module is duplicated (because of another dep) and it seems this makes all deps of reason-urql to be moved away in their node_modules folder...

Not everybody will take a few minutes (you know devs...) to investigate which date is causing issue by reading by hand the lock file... That's why I would encourage peerDep: this give a warning to the user. The doc can also be explicit. For reason-react-native we are making bindings that match the version number to make this simple & auto documented: reason-react-native 0.62.x works with react-native 0.62.x.

[kitten]

Oh, that does make sense! I went through a couple of possibilities. And if it's explicitly graphql that's duplicated then this can indeed happen.

There are multiple possible dependency trees but the gist of it will be that it'll look like this:

• graphql@A
• reason-urql
  • urql
  • @urql/core
  • graphql@B

I'd still say though that this is unexpected because of two reasons:

• Imports aren't zero-cost, so I'm very surprised that Bucklescript would inline them into your source, because that's not a safe assumption to make by it and I'd consider it a bug, since dependencies aren't always hoisted.
• The graphql package is never installed directly by urql or @urql/core but a peer dependency which does make this a mystery still.
• The range of graphql in all urql packages is extremely permissive. In fact it's ^0.11.0 || ^0.12.0 || ^0.13.0 || ^14.0.0 || ^15.0.0

It'd be interesting to see the output of yarn why and see whether a run of npx yarn-deduplicate && yarn fixes this.

I'll still look into adding a peer dependency, however, we have also never done this because reason-urql is (was?) behind on which urql version it depended on.

[parkerziegler]

👋 Hey folks, the fix for this has been merged into main and released on v3.0.0-rc.0. Docs are in the README for installing urql and its peerDependencies as part of the installation for your project. Thanks!