Apollo-link: zen-observable-ts combines 'export' and 'require'

[acoreyj]

Expected Behavior
Can package zen-observable-ts with Rollup

Actual Behavior
zen-observable-ts results in
ReferenceError: require is not defined

This is because the compiled zenObservable.js is

export var Observable = require('zen-observable');

As you can see here https://github.com/rollup/rollup/issues/1058#issuecomment-254187433 you shouldn't combine export and require in the same file as commonjs will ignore the file if it has export/import.

It should compile to something like

import * as zenObservable from 'zen-observable';
export var Observable = zenObservable;

[acoreyj]

added PR that fixes the issue

[evans]

@acoreyj Thanks for opening the PR and Issue! I'm not sure how to rectify this, since using the import syntax will break React Native. See #389, #248, and #515. Do you have an example project that I can clone?

[acoreyj]

@evans
Yup I did just run into #248 with my fix. I updated my PR to what I think is an actual fix. Basically we need to export the default of zen-observable.

I created a repo showing this error here https://github.com/acoreyj/apollo-link-observable-rollup-issue

See the readme for more details.

Definitely some strange behavior here, my guess is stemming from how zen-observable handles it's exports? Possibly fixed in most recent version?

[gaberudy]

This is definitely an issue and I am experiencing it myself with the rollup builds.

This is very painful, hopefully this PR can be merged in.

[acoreyj]

in the Pull Request I added the allowSyntheticDefaultImports to the tsConfig so now the fixed zenObservable.js will look more normal

Note I haven't tested this change with react-native but it definitely fixes #248

import zenObservable from 'zen-observable';
export var Observable = zenObservable;

[bennypowers]

Anything I can do to help this along?

[acoreyj]

@bennypowers

Not sure what else can be done, repo to reproduce is here and PR to fix is here

In the past they mentioned switching to rxjs which would resolve this problem, perhaps that is being actively worked on.

[evans]

@acoreyj @bennypowers Thank you for the help so far! I've been chasing after some server changes. I believe the changes in #559 will work for rollup and probably react native.

I think what would help is adding a simple test to bundle zen-observable-ts with rollup. Part of my hesitation is being burned by these types of changes in the past without being able to understand what is going to break. A test would ensure that we don't break things in the future. Does a test like this sound feasible?

[acoreyj]

@evans

I looked into it and it doesn't seem very practical to test for since the error is at runtime due to the require statement. I think the test would have to use the compiled bundle and then use some sort of headless browser to test. It seems like the Jest test handles the import fine regardless of what method is used.

I'm no Jest expert though so maybe someone else would have some input on something like this, I couldn't figure it out though.

[bennypowers]

@acoreyj I think you hit the nail on the head there. This is about (trying and ultimately failing at) running apollo in the browser. A test using headless chrome and selenium or something like cypress is what's needed here.

[evans]

@acoreyj Thanks for bringing up the issue originally!

@bennypowers I think there would be a way to test the bundles for no requires by removing it from the global scope with something like delete global.require, similar to how we do the check for no fetch in this http-common test. If that makes sense, would really appreciate a change to add a test!

Once CI passes, I'll be able to publish

[arjunyel]

Any chance once this lands you can also add the latest version of this to apollo-boost? If you want an error reproduction: https://github.com/apollographql/apollo-client/issues/3276