Two bugs in 1.7.0 due to including graphql dependency.

[kentcdodds]

This is copy/pasted from a reproduction repository you can checkout :)

graphql-request-1.7.0-bug

This is a reproduction of a bug that came up in 1.7.0.

There are actually 2 bugs IMO.

Bundle Size

In 1.7.0, graphql-tag was added as a dependency and graphql was added to peerDependencies. One of the reasons we use graphql-request is because it's so small.

Here's the webpack output for graphql-request@1.6.0:

Hash: 00ae46853441bb94f0bc
Version: webpack 4.16.2
Time: 637ms
Built at: 2018-07-24 16:55:50
  Asset      Size  Chunks             Chunk Names
main.js  12.6 KiB       0  [emitted]  main
Entrypoint main = main.js
[1] ./src/index.js 25 bytes {0} [built]
    + 3 hidden modules

Here's the webpack output for graphql-request@1.7.0:

Hash: 7489bc885808f855eac6
Version: webpack 4.16.2
Time: 2910ms
Built at: 2018-07-24 16:54:42
  Asset     Size  Chunks             Chunk Names
main.js  156 KiB       0  [emitted]  main
Entrypoint main = main.js
[2] ./src/index.js 25 bytes {0} [built]
[5] ./node_modules/graphql/index.mjs + 99 modules 474 KiB {0} [built]
    |    100 modules
    + 4 hidden modules

Notice the difference in the main.js filesize: 12.6 KiB before, 156 KiB after. And this is using webpack --mode production. This is definitely a bug.

You can also checkout the built files:

• 1.6.0
• 1.7.0

webpack not handling a require statement

I'm honestly not sure what's up here, but if you open up the index.html in 1.6.0 and open the console there's nothing in there and the code in graphql-request runs fine.

If you open up the index.html in 1.7.0 there's the following error in the console:

main.js:1 Uncaught ReferenceError: require is not defined
    at Object.<anonymous> (main.js:1)
    at t (main.js:1)
    at Module.<anonymous> (main.js:1)
    at t (main.js:1)
    at Object.<anonymous> (main.js:1)
    at t (main.js:1)
    at Module.<anonymous> (main.js:1)
    at t (main.js:1)
    at main.js:1
    at main.js:1

I'm not 100% certain why that's happening, but it's this statement:

require("./../../process/browser.js");

I think it's in the graphql/jsutils/instanceOf.mjs file where it's using process. I'm not sure why that's causing issues here...

That said, if the first bug is fixed (remove the dependency on graphql), then this bug will also be fixed.

Thanks!

[divyenduz]

Thanks for reporting this. Related issue: graphql/graphql-js#1272

@timsuchanek : We need just the print function from graphql-js, extracting that inline will remove the graphql dependency and reduce bundle size. Sounds good?

I will also add bundle size check in CI.

[kentcdodds]

I'm happy with that so long as the bundle size stays below 15kb 👍 thanks!

[divyenduz]

Added bundlesize to CI here: #96
It is still under 3 KB, of course it is not counting the peer dep size.

Is it possible for you to configure webpack to not include unused code from peer dep?

[kentcdodds]

Is it possible for you to configure webpack to not include unused code from peer dep?

What do you mean? This line is importing the peer dep, so that code is used (though the module may be tree-shaken, that's still a LOT of code). I'm pretty certain that the feature must be dropped entirely. If someone wants that feature then they can use one of the many other libraries that provide this capability... The appeal of graphql-request is its size. If this isn't changed, then I'll probably look for another module or fork this one if I ever need changes, which is fine, I just thought that the goal of this module was to fill the needs of size-conscious people like me.

[divyenduz]

It is definitely a goal to maintain the tiny size of this project. Which is why I want to work with you and reliably detect the impact of any dependencies change in CI.

Currently, the bundlesize check is CI passes. Also, the size reported by bundlephobia is under 15 KB as you had originally asked for.

[kentcdodds]

Interesting! I'll give it a look. Thanks!

[kentcdodds]

Hmm... I updated the example to use 1.8.0 and the bundle size is still the same and I'm still getting the console error.

Maybe there's a bug in bundlephobia. The fact is that graphql-request is using print from graphql which is what's causing the issues. If I remove the line import { print } from 'graphql' then the size goes down to 12.6 KiB.

I don't think it's possible to import print from graphql and keep the filesize down because print requires a LOT of code. And it's impossible to tree-shake that because it's used in the source code. I mean, you could restructure the code so folks who want that support can import another file or something, but I think that would be pointless because people who use that don't care about size and can therefore use another package altogether.

[divyenduz]

@kentcdodds: Thanks for working with me on this one and providing the reproduction. This PR reverts this feature: #97

Further actions for me:-

1. • Create issue for reliably detecting bundle size in CI to avoid this regression in future, both bundlephobia and bundlesize didn't work for graphql being a peer dependency.
2. • Reopen the issue Support for parsed queries (AST) #10

Thanks!

[divyenduz]

This is release in 1.8.1 🎉

[kentcdodds]

Super! Verified with my reproduction and it's back down to 12.6 KiB. Thank you!