Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document that Symbol polyfill is required to use in React Native/old browsers #535

Closed
ManAnRuck opened this issue Dec 15, 2017 · 12 comments
Closed
Labels
docs Focuses on documentation changes enhancement help wanted Extra attention is needed

Comments

@ManAnRuck
Copy link

I try to mocking queries on client side (for developing).
it works on web and iOS. on android I get the following message only by adding this line in code without other changes.

import { makeExecutableSchema, addMockFunctionsToSchema } from 'graphql-tools';
bildschirmfoto 2017-12-15 um 18 09 14

<unknown>
    errors.js:13:26
loadModuleImplementation
    require.js:191:12
guardedLoadModule
    require.js:138:36
_require
    require.js:118:20
<unknown>
    defaultMergedResolver.js:5:23
loadModuleImplementation
    require.js:191:12
guardedLoadModule
    require.js:138:36
_require
    require.js:118:20
<unknown>
    makeRemoteExecutableSchema.js:44:38
loadModuleImplementation
    require.js:191:12
guardedLoadModule
    require.js:138:36
_require
    require.js:118:20
<unknown>
    index.js:3:43
loadModuleImplementation
    require.js:191:12
guardedLoadModule
    require.js:138:36
_require
    require.js:118:20
<unknown>
    index.js:8:17
loadModuleImplementation
    require.js:191:12
guardedLoadModule
    require.js:138:36
_require
    require.js:118:20
<unknown>
    client.js:5
loadModuleImplementation
    require.js:191:12
guardedLoadModule
    require.js:138:36
_require
    require.js:118:20
<unknown>
    index.js:4
loadModuleImplementation
    require.js:191:12
guardedLoadModule
    require.js:138:36
_require
    require.js:118:20
<unknown>
    App.js:3
loadModuleImplementation
    require.js:191:12
guardedLoadModule
    require.js:138:36
_require
    require.js:118:20
<unknown>
    index.android.js:1
loadModuleImplementation
    require.js:191:12
guardedLoadModule
    require.js:131:45
_require
    require.js:118:20
global code
    ```

here is a link to the branch to try it
https://github.com/demokratie-live/democracy-client/tree/GraphQL
@stubailo
Copy link
Contributor

Looks like RN doesn't have Symbol: https://github.com/apollographql/graphql-tools/blob/3e4b61a6111ecb724efab5668115cdee08563057/src/stitching/errors.ts#L4

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Looks like this is an issue with React Native: facebook/react-native#4676

A fix is suggested here: facebook/react-native#4676 (comment)

Maybe we can just add docs for the relevant polyfill? It's supported in all modern browsers so it seems silly to avoid using Symbol just for React Native.

@stubailo stubailo changed the title [android] Can't find variable: Symbol Requires Symbol polyfill to use in React Native Dec 15, 2017
@stubailo stubailo added docs Focuses on documentation changes question labels Dec 15, 2017
@kbrandwijk
Copy link
Contributor

That fix introduces other errors, as mentioned in the same thread. Those issues are still not fixed. Since RN is notioriously slow in fixing stuff, I'm afraid the solution needs to be on the side of graphql-tools.

@stubailo
Copy link
Contributor

Is there any way to polyfill Symbol in React Native? That would definitely be my preference.

We could certainly stop using it in this package, but I'd really prefer to be able to rely on modern JavaScript stuff if we can. Maybe let's take a day to look for that solution, and then we can maybe have a PR to remove Symbol if we don't find any external solution.

@giautm
Copy link
Contributor

giautm commented Dec 17, 2017

Here is a polyfill can be use https://github.com/seanmonstar/symbol/blob/master/README.md

@stubailo
Copy link
Contributor

@kbrandwijk I think that bug has been fixed: facebook/react-native#11968

@stubailo
Copy link
Contributor

Can anyone try the fix on React Native, and see if it resolves the issue? If so, we can add a section to the docs.

@giautm
Copy link
Contributor

giautm commented Dec 18, 2017

Yep, it works on React-Native 0.51. https://snack.expo.io/@giautm/symbol-test

@ReggaePanda : which version of react-native are you using?

@kbrandwijk
Copy link
Contributor

@stubailo stubailo changed the title Requires Symbol polyfill to use in React Native Document that Symbol polyfill is required to use in React Native/old browsers Dec 18, 2017
@stubailo
Copy link
Contributor

Awesome, someone want to send a PR here? Right under the install directions I'd put a small "polyfills" heading: https://github.com/apollographql/graphql-tools/blob/master/docs/source/index.md

@ManAnRuck
Copy link
Author

ManAnRuck commented Dec 21, 2017

@giautm this is the output from react-native info

Environment:
  OS: macOS High Sierra 10.13.2
  Node: 9.3.0
  Yarn: 1.3.2
  npm: 5.6.0
  Watchman: 4.9.0
  Xcode: Xcode 9.2 Build version 9C40b
  Android Studio: 3.0 AI-171.4443003

Packages: (wanted => installed)
  react: 16.0.0 => 16.0.0
  react-native: 0.51.0 => 0.51.0

@stubailo stubailo added enhancement help wanted Extra attention is needed and removed question labels Dec 21, 2017
@wKovacs64
Copy link

@stubailo Symbol polyfills conflict with React Native's own Set polyfill, leading to some really obscure and hard-to-troubleshoot bugs. For example, polyfilling Symbol in a fresh RN project (0.51.0, 0.52.2, and 0.53.0-rc.0) results in the following error on Android:

Invariant Violation: Objects are not valid as a React child (found: object with keys {$$typeof, type, key, ref, props, _owner, _store}). If you meant to render a collection of children, use an array instead.

(Note: it works in Expo, so they must be doing something to accommodate.)

It would be a lot less painful for RN developers (which I would think make up a large portion of Apollo users, given GraphQL's benefits on mobile) if the use of Symbol was temporarily removed from graphql-tools until RN has resolved the Symbol situation (although I completely understand why you'd prefer an alternative solution - I would too 🤷‍♂️ ).

@freiksenet
Copy link
Contributor

Should be fixed in next version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Focuses on documentation changes enhancement help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants