Make WebsocketLink headers work the same as AuthLink

[fermuch]

Description

Before this change, the function was returning an object of the type { Authorization: any }, which is incompatible with what is expected from connectionParams. This change uses the same methods as they are used on AuthLink.

Tests are not passing but it seems to be unrelated to this change (errors come from CacheHelpersTest.ts)

Checklist

• npm test passes
• npm run build works

[wtrocki]

ping @darahayes

[wtrocki]

Thank you so much for contribution. I will check why tests are failing and merge this PR as soon as possible

[fermuch]

Thanks @wtrocki !

May I add the question: where is token from AuthContextProvider used? I could not find what it should have nor where it is being used, but providing only header seems to be enough for auth purposes.

[darahayes]

Hey @fermuch I've done a little bit of work around the WebsocketLink and the AuthContextProvider. To answer your question about token, I would say it's a leftover artefact from the previous project that parts of offix were extracted from and it probably needs to be cleaned up. In fact, the whole area will probably need to be refactored at some point. So, thanks for your contribution, it puts us in the right direction. I'll try to get this merged today.

[darahayes]

Before this change, the function was returning an object of the type { Authorization: any }, which is incompatible with what is expected from connectionParams

Are you sure this is definitely incompatible with what is expected from connectionParams or is it possible that your application is expecting something different?

Documentation for connectionParams says you should return an object with any values you want/need in there. Additionally, the types show it as:

export type ConnectionParams = {
  [paramName: string]: any,
};

My understanding is this, the original code is definitely making some bad assumptions but I think we also do not need to explicitly return an object with the headers key in connection params.

I think instead we should refactor the AuthContext interface to look something like this:

export interface AuthContext {
  headers: {
    [headerName: string]: any
  }
}

Here we would rename header to headers and now it becomes a lot clearer and more obvious for the end user, of what you should pass. Also, there's no more token.

And then in the the WebsocketLink we would have:

connectionParams: async () => {
  if (userOptions.authContextProvider) {
    const { headers } = await userOptions.authContextProvider();
    return headers
  }
},

and then the AuthLink would become:

export const createAuthLink = (config: OffixClientConfig): ApolloLink => {
  const asyncHeadersLink = setContext(async (operation, previousContext) => {
    if (config.authContextProvider) {
      const { headers } = await config.authContextProvider();
      return {
        headers
      };
    }
  });
  return asyncHeadersLink;
};

@fermuch can you try out that code and see if it works for you and if it does, then we would be really happy to accept those changes.

[wtrocki]

@darahayes Let's make suggested changes on top of this PR and we can integrate it for this release.

Here we would rename header to headers and now it becomes a lot clearer and more obvious for the end user, of what you should pass. Also, there's no more token.

Let's do it as we going to have now breaking release so no worries with introducting this change. Big +1 on any cleanup from our side

[fermuch]

Are you sure this is definitely incompatible with what is expected from connectionParams or is it possible that your application is expecting something different?

connectionParams accept any, but returning {Authorization: any} makes no sense (since Authorization isn't used by connectionParams, it should be a header)

I think instead we should refactor the AuthContext interface to look something like this:

I completly agree! But wouldn't dropping header and token be a breaking change? Is that acceptable?

Let's do it as we going to have now breaking release so no worries with introducting this change. Big +1 on any cleanup from our side

Got it!

[darahayes]

connectionParams accept any, but returning {Authorization: any} makes no sense (since Authorization isn't used by connectionParams, it should be a header)

I agree we shouldn't explicitly have Authorization as a property in there, this is something that should be set by the user if they need it.

I'm also +1 to the breaking change because we're putting out a release with breaking changes soon.

[fermuch]

I've applied the changes proposed by @darahayes and tested locally on my app.

[darahayes]

Awesome, thanks so much @fermuch!