3.1.0-pre.0 bundle size regression with rollup

[henryqdineen]

I know this is prerelease but I just want to give some feedback. This is the same as a comment I made in faa64b3#r40855898 but I created an issue for more visibility.
I tried out 3.1.0.pre.0 and we are seeing a bundle size regression in our rollup bundle. Here is a snippet from dist/core/index.js before and after

export { ApolloClient, } from './ApolloClient';
export { ObservableQuery, } from './ObservableQuery';
export { NetworkStatus } from './networkStatus';
export * from './types';
export { isApolloError, ApolloError } from '../errors';

import '../utilities/graphql/directives.js';
import '../utilities/graphql/fragments.js';
export { isReference, makeReference } from '../utilities/graphql/storeUtils.js';
import '../utilities/graphql/getFromAST.js';
import '../utilities/graphql/transform.js';
export { default as Observable } from 'zen-observable';
import '../utilities/common/mergeDeep.js';
import '../utilities/common/cloneDeep.js';
import '../utilities/common/maybeDeepFreeze.js';

I don't think rollup is able to treeshake the imports with no specifiers. Please let me know if you need any more info if there are any workarounds.

[henryqdineen]

My current workaround is just to strip these imports off with a rollup plugin. Would that cause any issues and would it make sense to add something similar to resolveESMImportsAndFileExtensions()?

{
  transform(code, id) {
    const magicString = new MagicString(code);
    const ast = this.parse(code);

    walk(ast, {
      enter: function (node) {
        if (
          node.type === "ImportDeclaration" &&
          node.specifiers.length === 0 &&
          node.source.value !== "symbol-observable"
        ) {
          magicString.remove(node.start, node.end);
        }
      },
    });

    return {
      code: magicString.toString(),
    };
  },
};

[benjamn]

@henryqdineen This is great feedback, and really validates the decision to publish -pre.0 before 3.1.0!

I was uneasy with those specifier-less imports too, and couldn't find a combination of Rollup options to prevent them. I think they have something to do with import hoisting, which is supposedly disabled when preserveModules: true is set (which we do), but apparently not? Ironically, Rollup seems to be creating a problem that it can't solve (tree-shake) later.

In any case, I am planning to replace resolveESMImportsAndFileExtensions with a step that simply resolves all existing module identifiers, without injecting any new imports or exports.

[henryqdineen]

Thanks! Feel free to reach out if you need help testing out alternate solutions.

[benjamn]

@henryqdineen Give @apollo/[email protected] (which includes #6694) a try when you have a chance!

[henryqdineen]

Thanks @benjamn! Nice solution in #6694. I just checked it out 3.1.0-pre.2 and it does seem to treeshake much better but I did notice one thing. It's having trouble treeshaking graphql-tag, which we wish to omit to reduce bundle size (we want to parse the graphql at build time). I think it has something to do with this no longer being in index.js:

export { default as gql } from 'graphql-tag';
export { disableExperimentalFragmentVariables, disableFragmentWarnings, enableExperimentalFragmentVariables, resetCaches } from './core/index.js';

Instead we have:

export * from "./core/index.js";

And in core/index.js:

import gql from 'graphql-tag';
export var resetCaches = gql.resetCaches, disableFragmentWarnings = gql.disableFragmentWarnings, enableExperimentalFragmentVariables = gql.enableExperimentalFragmentVariables, disableExperimentalFragmentVariables = gql.disableExperimentalFragmentVariables;
export { gql };

My assumption is that rollup is considering stuff like gql.resetCaches a potential side effect. We can figure out a workaround but definitely let us know if you have any ideas or if I'm missing anything obvious. If there is a future where using the graphql-tag at runtime is mandatory or if you think parsing at build time might be an anti-pattern I'd love to hear more about it.

[henryqdineen]

I noticed one other small regression from 3.0.0 regarding treeshaking. We aren't using subscriptions but I noticed that 3.1.0-pre.2 would cause SubscriptionData to be bundled. I think this is more of a issue with rollup and the way TypeScript transforms classes but I wanted to mention it anyway.

In 3.0.0:

apollo-client/src/react/hooks/utils/useBaseQuery.ts

Line 10 in 8363f90

import { QueryData } from '../../data/QueryData';

In 3.1.0-pre

apollo-client/src/react/hooks/utils/useBaseQuery.ts

Line 10 in c4fbe44

import { QueryData } from '../../data';

[benjamn]

@henryqdineen Do you think it would help to replace

import gql from 'graphql-tag';
export var resetCaches = gql.resetCaches, disableFragmentWarnings = gql.disableFragmentWarnings, enableExperimentalFragmentVariables = gql.enableExperimentalFragmentVariables, disableExperimentalFragmentVariables = gql.disableExperimentalFragmentVariables;
export { gql };

with

export { default as gql } from 'graphql-tag'

or is it more a matter of declaring the graphql-tag package as not having side-effects?

[henryqdineen]

@benjamn I just tried it out by adding "sideEffects": false to node_modules/graphql-tag/package.json and that did the trick! Are there plans to add that flag to that project? The other solution would work also but seems like a breaking change since we are removing named exports. Thanks as usual!