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

fix: authMiddleware swallows other headers #8470

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

russell-dot-js
Copy link
Contributor

@russell-dot-js russell-dot-js commented Jun 1, 2023

RedwoodApolloProvider allows specifying a graphQLClientConfig. One of these options allows adding links to those provided with redwood via a function, e.g.

link: (rwLinks) => ApolloLink.from([myAwesomeLinkWithACustomHeader, ...rwLinks])

The problem is that the "AuthMiddleware" provided by redwood does not forward any other non-auth related headers, so your only option to add headers are static headers that can be defined via httpLinkConfig.

The temporary workaround is:

link: (rwLinks) => {
  // this will break if redwood changes the order of their links!
  // their "authMiddleware" link is broken and removes all headers
  // that they don't supply. so we want to make sure to insert after
  // them, currently in index 1
  const firstTwo = rwLinks.slice(0, 2);
  const rest = rwLinks.slice(2);

  return ApolloLink.from([...firstTwo, myAwesomeLinkWithACustomHeader, ...rest])
},

Since "AuthMiddleware" is not exported nor does it have any static properties that can be used to determine which link it is, knowledge of the ordering of redwood's internal links array needs to be baked into our custom apollo config.

Gross

@jtoar jtoar added the release:fix This PR is a fix label Jun 1, 2023
@replay-io
Copy link

replay-io bot commented Jun 1, 2023

19 replays were recorded for 1c1d9fc.

image 0 Failed
image 19 Passed

View test run on Replay ↗︎

Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @russell-dot-js; we'll get this fix out, and we realize the API for configuring the link could be easier to work with. If you have a suggestion we're happy to consider it

@jtoar jtoar added the fixture-ok Override the test project fixture check label Jun 1, 2023
@jtoar jtoar merged commit 300c358 into redwoodjs:main Jun 1, 2023
7 checks passed
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jun 1, 2023
@russell-dot-js
Copy link
Contributor Author

russell-dot-js commented Jun 2, 2023

Thanks @russell-dot-js; we'll get this fix out, and we realize the API for configuring the link could be easier to work with. If you have a suggestion we're happy to consider it

Thanks for the fast merge @jtoar!
Here are a couple ways to make it easy to configure the link:

  1. Export the links
// packages/web/src/apollo/index.tsx

// before
//  const authMiddleware = new ApolloLink((operation, forward) => {

// after
export const authMiddleware = new ApolloLink((operation, forward) => {
// in my redwood app:

link: (rwLinks) => ApolloLink.from(rwLinks.filter(link => link !== authMiddleware))

  1. Add a static property to the links so they can be introspected
// packages/web/src/apollo/index.tsx

const authMiddleware = new ApolloLink((operation, forward) => {
  // impl
});

// add this (typescript won't be happy)
authMiddleware.__rw_link_name = 'authMiddleware'
// in my redwood app:

link: (rwLinks) => ApolloLink.from(rwLinks.filter(link => link.__rw_link_name !== 'authMiddleware'))

Second one is gross IMO, first one is simple. Happy to PR whichever you're interested in

@jtoar jtoar modified the milestones: next-release, next-release-patch Jun 2, 2023
@jtoar jtoar modified the milestones: next-release-patch, v5.2.4 Jun 2, 2023
@jtoar jtoar modified the milestones: v5.2.4, next-release, next-release-patch Jun 2, 2023
@jtoar
Copy link
Contributor

jtoar commented Jun 2, 2023

@russell-dot-js exporting the links sounds reasonable. Actually I kind of remember talking about this API at least a year ago, and I think there was a gotcha with exporting some of them because some of them use React Context (specifically the authMiddleware link and the httpLink link depend on the FetchConfigProvider). Maybe it could still be done, but they wouldn't be initialized till the RedwoodApolloProvider renders. Do I have the details there right, and is there still a cocnern there if so?

Another idea: instead of passing an array to the link function, we could pass an object so you can grab the links by name. I think we went with an array so that they'd be in the right order already. But we could provide that information via documentation.

Lastly @russell-dot-js, I just released this fix in v5.2.4.

@russell-dot-js
Copy link
Contributor Author

@russell-dot-js exporting the links sounds reasonable. Actually I kind of remember talking about this API at least a year ago, and I think there was a gotcha with exporting some of them because some of them use React Context (specifically the authMiddleware link and the httpLink link depend on the FetchConfigProvider). Maybe it could still be done, but they wouldn't be initialized till the RedwoodApolloProvider renders. Do I have the details there right, and is there still a cocnern there if so?

Another idea: instead of passing an array to the link function, we could pass an object so you can grab the links by name. I think we went with an array so that they'd be in the right order already. But we could provide that information via documentation.

Lastly @russell-dot-js, I just released this fix in v5.2.4.

Thanks @jtoar! I'll check out your concerns and see if exporting the links is a nonstarter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixture-ok Override the test project fixture check release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants