apollographql / meteor-integration

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

User Accounts middleware is not working on IE11 #73

Closed ecerroni closed 7 years ago

ecerroni commented 7 years ago

I have this code for my mutation. Everything is working perfectly everywhere but in IE11. It throws an error because there in no context.

      Mutation: {
        insertBusinessIdea (_, {name}, context) {

          if (!context.userId) {
            throw new Meteor.Error('User must be logged in to create a new Business Idea') //IE11 not working
          }

          const businessIdeaId = BusinessIdeas.insert({
            name: name,
            createdBy: context.userId,
            createdAt: new Date()
          })
          return BusinessIdeas.findOne(businessIdeaId)
        }
xavxyz commented 7 years ago

Hey @ecerroni!

Could you paste here the error you have server-side? That's weird, because the context is only a server-side thing for the resolvers: http://dev.apollodata.com/tools/graphql-tools/resolvers.html#Resolver-function-signature

Assumption: could it be a problem of localstorage and IE11 (the browser local storage is where Meteor stores the login token, and the apollo package grabs it and send it with the queries)? I found this issue about localstorage & IE11 on StackOverflow: http://stackoverflow.com/questions/21155137/javascript-localstorage-object-broken-in-ie11-on-windows-7

ecerroni commented 7 years ago

@xavcz

Sorry for the delay of my answer. I wanted to make sure I can add more meaningful information.

First: This happens only in production. When I run Meteor with the apollo integration on localhost everything is fine, including IE11 When I build the bundle and deploy the production version on the server that issue happens though exclusively when using IE11

I console logged the context object on the server and for each request its empty {}

So I can log into the Meteor app though apollo context === {}

First two console.logs are by IE11, the last one is from Chrome

Service: AWS
Service: [azureAd]
All Auth Services configured...
context List {}
context Insert {}
context List { userId: '5a5LkfuoKdaJTZheG',
  user: 
   { _id: '5a5LkfuoKdaJTZheG',
     createdAt: Tue Feb 14 2017 10:46:36 GMT+0000 (UTC),
     services: 
      { password: [Object],
        azureAd: [Object],
        email: [Object],
        resume: [Object] },
     username: '9B5SftNRmRrcdmq7D',
     emails: [ [Object] ],
     profile: 
      { firstname: 'Enrico',
        lastname: 'Cerroni',
        displayName: 'Enrico Cerroni',
        seenSteps: [Object] } } }

Second: I had additional problems that might be related or could give you some hints.

I had to change this code:

const withGetCurrentToken = graphql(GET_CURRENT_TOKEN, {
  props: ({ data: { error, loading, getCurrentToken } }) => {
    if (loading) return { dataLoading: true }
    if (error) return { hasErrors: true }
    return {
      currentToken: getCurrentToken.currentToken,
      allTokens: getCurrentToken.allTokens
    }
  },
  options: (ownProps) => (
    { forceFetch: true, variables: { id: ownProps.userId} }
  )
})

Into this:

const withGetCurrentToken = graphql(GET_CURRENT_TOKEN, {
  props: ({ data: { error, loading, getCurrentToken } }) => {
    if (loading) return { dataLoading: true }
    if (error) return { hasErrors: true }
    return {
      currentToken: getCurrentToken && getCurrentToken.currentToken,
      allTokens: getCurrentToken && getCurrentToken.allTokens
    }
  },
  options: (ownProps) => (
    { forceFetch: true, variables: { id: ownProps.userId} }
  )
})

Otherwise IE11 would throw this type of error: Unable to get property 'getCurrentToken' of undefined or null reference This is happening again only for IE11 and only if on production

I hope you have a solution for this.

xavxyz commented 7 years ago

Ouf, context is defined, it's just empty! I thought it was worse! 😅

So ok, it looks like on Chrome everything works fine?

When I run Meteor with the apollo integration on localhost everything is fine, including IE11

+

When I build the bundle and deploy the production version on the server that issue happens though exclusively when using IE11

What you are saying reminds me troubles many folks have because of not setting correctly the ROOT_URL of your Meteor app.

➡️ Did you specify your ROOT_URL in the env variables when deploying?

I had additional problems that might be related or could give you some hints.

What about grabbing networkStatus when you destructure data and logging its value (https://github.com/apollographql/apollo-client/blob/master/src/queries/networkStatus.ts)?

If you have a warning saying that getCurrentToken doesn't have any property, I believe it means that your query failed.

➡️ Maybe because you depend on context.userId or context.user in your resolver?

🤓 Many assumptions there, do you have a reproduction repo? That would be a lot easier! Keep us tuned about your progress!

ecerroni commented 7 years ago

If you have a warning saying that getCurrentToken doesn't have any property, I believe it means that your query failed.

:arrow_right: Maybe because you depend on context.userId or context.user in your resolver?

Yes, that's exactly the case. I depend on it in my resolver.

The ROOT_URL has been already specified.

I grabbed and logged networkStatus. I get 7 everywhere, including production with IE11

xavxyz commented 7 years ago

So what's your resolver? You could make it return an error if you don't have context.user(Id)?

What is Accounts._storedLoginToken() returning client-side in IE11 production?

ecerroni commented 7 years ago
if (!context.userId) {
            throw new Meteor.Error('User must be logged in to create a new Business Idea')
          }

I just check it here. If user is not logged in then he cannot create a new Item. Though he's actually logged in my case.

xavxyz commented 7 years ago

Oh my bad, forgot about your first comment with the code.

But it's a mutation resolver, that doesn't have anything to do with the query get current token, does it?

For the context, I would recommend to clone this package and check what the request looks like server side

ecerroni commented 7 years ago

Yeah, sure. I have that context check on each resolver so I copied the first one :)

IE11 production actually returns a token from localstorage: Accounts._storedLoginToken() "6LaOPLueBbby7hwBOQyUMo5kkv6p98cQbU0yMr89k99"

ecerroni commented 7 years ago

It might be worth saying that on production it's deployed as a docker container behind an nginx reverse proxy.

xavxyz commented 7 years ago

Ok, so that was just to know if the login token was really set, but you told it, the user is logged in.

My new assumption is that Headers don't work on IE: https://developer.mozilla.org/en-US/docs/Web/API/Headers

This is what we use to plug in the token to the request headers, in main-client.js.

The thing I don't get is why it's working on dev environment.

xavxyz commented 7 years ago

So as a hypothetic solution for your problem:

So options.headers should exist and the login token should be attached to options.headers.Authorization 🤔

I'm on a Mac and don't have any VM to run Windows at the moment, could you log what's the value of options.headers with and without this modification?

Thanks! 👍

ecerroni commented 7 years ago

I think I lost you :) So

clone the package locally in a /packages folder

/packages is in the app root folder or in the .meteor folder

edit main-client.js and on top of the file, import 'isomorphic-fetch';

Which main-client.js? The one under the app '/client' folder?

So options.headers should exist

Should exist where exactly? :)

xavxyz commented 7 years ago

No bother!

/packages in the app root folder 👍 This will enable a local version of the apollo package that you can hack to test if everything works!

Then you'll need to edit main-client.js of the apollo package 😌 It will be located in /packages/meteor-integration/main-client.js (it's this file: https://github.com/apollographql/meteor-integration/blob/master/main-client.js).

By importing isomorphic-fetch, my assumption is that it will make IE11 able to recognize Headers.

So that request.options.headers will actually be created (see code), and so the currentUserToken will be attached to the request.options.headers as the Authorization key, which will then be interpreted server-side to populate the context with user and userId (see code).

If it works, we can make a pull request to this package so nobody should encounter this problem again 🎉 :octocat:

Does it make more sense? Please tell me if it's unclear 🙆‍♂️

Note: btw there is a codetour of the package breaking it down in several parts to make it easy to understand: https://www.codetours.xyz/tour/xavcz/meteor-apollo-codetour 📖 🤖

xavxyz commented 7 years ago

Hey @ecerroni 👋 I believe there is an open PR that could fix your issue.

Could you try it and tell us what's up?

To try it, from your app root folder:

# if there isn't a /packages folder in your root directory, create one before doing these commands
cd packages
git clone https://github.com/xavcz/meteor-integration.git
cd meteor-integration
git checkout meteor-accounts-tests

All set! Then you can run / deploy your app as usual, the app will use the local package meteor/apollo from /packages/meteor-integration folder instead of the one from Atmosphere.

ecerroni commented 7 years ago

I'll try that. However in the meantime I had @serkandurusoy jump in on the project to figure out the issue I was facing.

He found out that IE11 was overriding the Bearer authorization token.

It happens only on certain intranet environment. That is somehow related to what you were already pointing at.

serkandurusoy commented 7 years ago

https://github.com/apollographql/meteor-integration/pull/74/files#r101972360