apollographql / meteor-integration

🚀 meteor add apollo
http://dev.apollodata.com/core/meteor.html
108 stars 45 forks source link

are you missing batchInterval? #78

Closed comus closed 7 years ago

comus commented 7 years ago

i mean

const interfaceArgument = {
    uri: config.uri,
    opts: config.opts,
    batchInterval: config.batchInterval     <------ this missing one
  }

see: https://github.com/apollographql/meteor-integration/blob/master/main-client.js#L43

otherwise Error: pollInterval must be a number, got undefined pollInterval error will get

comus commented 7 years ago

oh ,,, may be this is better

const interfaceArgument = {
    uri: config.uri,
    opts: config.opts
  }

  if (useBatchingInterface) {
    interfaceArgument.batchInterval = config.batchInterval
  }
lorensr commented 7 years ago

What version are you on? On Mon, Feb 27, 2017 at 12:08 AM Comus Leong notifications@github.com wrote:

i mean

const interfaceArgument = { uri: config.uri, opts: config.opts, batchInterval: config.batchInterval <------ this missing one }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apollographql/meteor-integration/issues/78, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPVmHTfzOvpvowk-GK8Bfzvz3GGoifmks5rglpGgaJpZM4MMswO .

comus commented 7 years ago

@lorensr the latest one

see: https://github.com/apollographql/meteor-integration/commit/e840181985787950235ad29317631b6304e7515a#diff-537547dc27e35b1053a83d76ce16acd8L40

@xavcz removed these code in above commit

comus commented 7 years ago

currently the code only check batchInterval is a number

and not pass it to create networkinterface

https://github.com/apollographql/meteor-integration/blob/master/main-client.js#L37

xavxyz commented 7 years ago

batchInterval is defined by default to 10 in the defaultNetworkInterfaceConfig 🤔 https://github.com/apollographql/meteor-integration/blob/master/main-client.js#L20-L23

And then merged with your customNetworkInterfaceConfig in config.

Is this issue a friendly call to "hey guys, something is missing"? 😌

Or do you have a bug in your app because of this? 😵 If yes, can you provide a reproduction please?

comus commented 7 years ago

hello guys, sorry for my limited english words, i know i must say more clearly.

what i mean is:

in @xavcz 's previous version

  const config = {
    ...defaultNetworkInterfaceConfig, 
    ...customNetworkInterfaceConfig,
  };

  // this will be true true if a BatchingNetworkInterface is meant to be used 
  // with a correct poll interval
  const useBatchingInterface = config.batchingInterface && typeof config.batchInterval === 'number';

  // allow the use of a batching network interface
  const interfaceToUse = useBatchingInterface ? createBatchingNetworkInterface : createNetworkInterface;

  // configure the (batching?) network interface with the config defined above
  const networkInterface = interfaceToUse(config);   <---- i know that you include everything config here

but lorensr version is different, the latest 9 hours ago.

the latest commit Don't pass extra attributes in the NetworkInterfaceOptions object

const config = {
    ...defaultNetworkInterfaceConfig,
    ...customNetworkInterfaceConfig,
  };

  // this will be true true if a BatchingNetworkInterface is meant to be used
  // with a correct poll interval
  const useBatchingInterface = config.batchingInterface && typeof config.batchInterval === 'number';

  // allow the use of a batching network interface
  const interfaceToUse = useBatchingInterface ? createBatchingNetworkInterface : createNetworkInterface;

  // http://dev.apollodata.com/core/apollo-client-api.html#NetworkInterfaceOptions
  const interfaceArgument = {
    uri: config.uri,        <--------------------- the final interfaceArgument only include uri
    opts: config.opts,     <---------------------- and opts, not batchInterval
  }

  // configure the (batching?) network interface with the config defined above
  const networkInterface = interfaceToUse(interfaceArgument);    <---------- here
comus commented 7 years ago

apollo 0.3.1 work great

https://github.com/comus/meteor-apollo-demo/tree/apollo-0.3.1

apollo 0.4.0 (the latest commit Don't pass extra attributes in the NetworkInterfaceOptions object)

https://github.com/comus/meteor-apollo-demo/tree/apollo-0.4.0 apolloclient has error

W20170227-18:43:23.094(8)? (STDERR) Error: pollInterval must be a number, got undefined
W20170227-18:43:23.094(8)? (STDERR)     at new HTTPBatchedNetworkInterface (/Users/chris/Desktop/meteor-apollo-demo/node_modules/apollo-client/apollo.umd.js:251:19)
W20170227-18:43:23.094(8)? (STDERR)     at createBatchingNetworkInterface (/Users/chris/Desktop/meteor-apollo-demo/node_modules/apollo-client/apollo.umd.js:329:12)
W20170227-18:43:23.095(8)? (STDERR)     at createMeteorNetworkInterface (packages/apollo/main-client.js:49:28)
W20170227-18:43:23.095(8)? (STDERR)     at meteorInstall.node_modules.meteor.apollo.main-client.js (packages/apollo/main-client.js:101:21)
W20170227-18:43:23.095(8)? (STDERR)     at fileEvaluate (packages/modules-runtime.js:197:9)
W20170227-18:43:23.095(8)? (STDERR)     at Module.require (packages/modules-runtime.js:120:16)
W20170227-18:43:23.096(8)? (STDERR)     at Module.Mp.import (/Users/chris/.meteor/packages/modules/.0.7.9.1xj0l67++os+web.browser+web.cordova/npm/node_modules/reify/lib/runtime.js:75:16)
W20170227-18:43:23.096(8)? (STDERR)     at meteorInstall.node_modules.meteor.apollo.main-server.js (packages/apollo/main-server.js:1:75)
W20170227-18:43:23.096(8)? (STDERR)     at fileEvaluate (packages/modules-runtime.js:197:9)
W20170227-18:43:23.097(8)? (STDERR)     at require (packages/modules-runtime.js:120:16)

how i fix it

add

if (useBatchingInterface) {
    interfaceArgument.batchInterval = config.batchInterval
  }

https://github.com/comus/meteor-apollo-demo/commit/df964da3019741d2186426a80bec3b9d61ba30e9

xavxyz commented 7 years ago

Ok great, thanks @comus!

Do you think you can open a pull request from your branch to fix this issue please? 🚀

xavxyz commented 7 years ago

Thanks @comus, high five ✋