-
Notifications
You must be signed in to change notification settings - Fork 59
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
Make ApolloInterceptor id requirement read-only #159
Make ApolloInterceptor id requirement read-only #159
Conversation
@SebastianOsinski: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calvincestari Is there any reason why we need this to be writable in the protocol? I'm in agreement with @SebastianOsinski on this one.
This change is correct, the ID does not need to be writable. I think the I was going to get to this PR later in my day to ensure it isn't a breaking change in any other way. |
Thanks for this contribution @SebastianOsinski. I was hoping we could change the interceptor properties to be constants too but you're correct, there may be some projects relying on them being mutable. As long as the interceptor IDs are set before creating the request chain then everything should continue to work as expected. I've just updated this PR with the latest changes from |
The gitleaks check seems to be holding this up; I'm going to go ahead and merge it anyways. |
As far as I understand, there is no real need for interceptors' ids to be mutable, at least not by the library itself. I decided to leave ids as
var
in all default interceptors though as they're randomly generated and maybe someone has a need to change them to something more descriptive.