Incompatibility with Rollup

[chriswep]

I was trying to use apollo-client with Ionic RC0 which uses rollup now. As it seems apollo-client does not work when using rollup (+ commonjs plugin):

Uncaught TypeError: Cannot read property 'HTTPFetchNetworkInterface' of undefined(anonymous function) @ batchedNetworkInterface.js:75

As far as i could analyze it, this is due to a circular dependencies, where networkInterface is importing from batchedNetworkInterface and back. So as it seems the build from apollo-client can not be consumed by rollup+commonjs. I'm not deep enough into this to assess if this is a problem in the code itself or just the build of apollo-client. in any case i think apollo should work together with this technologies.

Corresponding issue on ionic repo: ionic-team/ionic-framework#8400

[stubailo]

Hmm, I'm not sure if this is a bug in Rollup, or in Apollo Client. I'm pretty surprised at Ionic's decision to use Rollup by default in such a widely used tool.

For context, AC works with Browserify, Typescript, Webpack, and Meteor build systems with no changes, so it seems like a bug in Rollup or the CommonJS plugin.

[chriswep]

I share your thoughts about the ionic decision.

As far as I understood, it is a conscious move on part of rollup-commonjs to not support certain things. Again, i'm not an expert in this, but it seemed like circular dependencies is such a thing. It also sounded like making a project compatible with Rollup wouldn't be a bad idea anyway because it forces the code to be structured better and it makes it ready for tree shaking.

[helfer]

@metzc @stubailo FWIW, I'm in the middle of refactoring the network interface, when I'm done there won't be any circular dependency any more.

[chriswep]

@helfer is there an ETA? depending on that i'll have to look for a short-term workaround

[jonathanheilmann]

@metzc, did you have a look at a short-term workaround? I'm struggling at this point and looking for a (temporarily) solution.

[DxCx]

hi @metzc as far as i know, @helfer is done with networkInterface fix (correct me if im wrong), but the release is not out yet.
can you create and share a github with failing example so i can install manually built apollo-client and test and verify it's fine now or give a proper fix?

[chriswep]

@jonathanheilmann @DxCx @helfer
meanwhile i also fixed the source to make it work.
i just checked out the current apollo-client code which indeed fixes networkInterface as you said.
There is still one in data/store.ts which i now also fixed. I created a pull request for it (along with two other fixes, see PR #778).

[stubailo]

Thanks for the PR!

[voodooattack]

Can someone please reopen this? The same thing is happening with ObservableQuery and QueryManager. The circular dependency breaks the browser bundle.

[stubailo]

Can someone help resolve this, and also add a test we can run on CI to avoid it coming back?

[voodooattack]

@stubailo A good alternative is compiling to es6 and adding it to package.json via the jsnext:main/module entries.

This is the bug responsible: rollup/rollup-plugin-commonjs#105

If we eliminate that step from the compilation everything should work fine.

[stubailo]

Either one works for me, but we need to make sure we add a test so that we don't break it again in the future.

[voodooattack]

@stubailo Just tried creating a new TypeScript configuration for es6, I'm getting these errors:

voodooattack@ideapad:~/devel/apollo-client$ tsc -p tsconfig.es6.json 
src/ApolloClient.ts(16,1): error TS1202: Import assignment cannot be used when targeting ECMAScript 2015 modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.
src/ApolloClient.ts(17,1): error TS1202: Import assignment cannot be used when targeting ECMAScript 2015 modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.
src/ApolloClient.ts(18,1): error TS1202: Import assignment cannot be used when targeting ECMAScript 2015 modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.
src/core/ObservableQuery.ts(30,1): error TS1202: Import assignment cannot be used when targeting ECMAScript 2015 modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.
src/core/ObservableQuery.ts(31,1): error TS1202: Import assignment cannot be used when targeting ECMAScript 2015 modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.
....

Should I replace all the offending import statements, or are there any special considerations when dealing with import = require()?

Also, how do you suggest we implement the tests? Shall I add rollup as a dev-dependency?

[stubailo]

Feel free to replace the import statements as long as they work the same way! I'm not an expert on imports in TS.

And yes, adding rollup as a dev dependency and just creating a new script would be great. Similar to how we have the file size script.

[voodooattack]

@stubailo Well, this happens when I replaced all the import = require directives with import x from y:

 TypeError: lodash_assign_1.default is not a function

I've reverted my changes and I'm trying a different approach. It looks like TS compiles the files anyway despite the errors. Is it OK if I just make the npm script ignore the return code and continue?

Something like this:

{
    "compile": "tsc && (tsc -p tsconfig.es6.json || true)",
}

I can have it hide the errors if that's preferable.

[stubailo]

I mean, I suspect the output might not be valid ES2015 in that case, but if it works then great!

Have you tried import * as x from y?

[voodooattack]

@stubailo Yeah:

src/ApolloClient.ts(16,30): error TS2497: Module ''lodash.isundefined'' resolves to a non-module entity and cannot be imported using this construct.

I'll test with the invalid stuff, if it works then we can ignore the errors, since it wouldn't compile the es6 version if the commonjs version isn't compiling anyway.

[stubailo]

This is one of the things that makes me want to stop depending on lodash and just put those utilities in the code directly.

[voodooattack]

Well, there's this: https://www.npmjs.com/package/lodash-es, too bad it only exports es6 :)

[stubailo]

Hmm, now that I think of it - is it even worth having an ES6 module version of our dependencies don't have it? Users of the package will still need the commonjs plugin anyway.

[voodooattack]

Damn, that's right! I didn't think that one through.

Okay, so back to square one. What should I try now, remove the circular dependency?

Edit: That's totally beyond me at this point. I don't normally use TS and I'm completely lost.
Edit: Is there a way to separate the declaration from the definition and import the declaration instead?

[stubailo]

if you don't want to work on removing the circular dep, you could start by adding a failing rollup compilation test, which will make a fix much easier.

[voodooattack]

#900 is up, sorry if I missed anything, I'm not too used to contributing to projects other than mine yet.

[helfer]

Fixed in #1069.