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

Use reference-equality when filtering rules in buildSchemaFromSDL #1551

Conversation

abernix
Copy link
Member

@abernix abernix commented Sep 30, 2019

Similar in spirit to apollographql/apollo-server#3338.

The technique previously used for removing rules from the standard specifiedRules was leveraging a check on Function.prototype.name, rather than doing direct object equality. While that does generally work, thanks to the more recent standardization of Function.prototype.name, it still breaks down under some of the more aggressive minification techniques since a function's name is not guaranteed to remain the same.

Fixes: apollographql/apollo-server#3335

Similar in spirit to apollographql/apollo-server#3338.

The technique previously used for removing rules from the standard
`specifiedRules` was leveraging a check on `Function.prototype.name`, rather
than doing direct object equality.  While that does generally work, thanks
to the more recent standardization of `Function.prototype.name`, it still
breaks down under some of the more aggressive minification techniques since
a function's name is not guaranteed to remain the same.

Fixes: apollographql/apollo-server#3335
@abernix
Copy link
Member Author

abernix commented Sep 30, 2019

@trevor-scheer Should I manually add an entry to the root CHANGELOG.md? Done.

@abernix abernix changed the title Use object equality when filtering rules in buildSchemaFromSDL Use reference-equality when filtering rules in buildSchemaFromSDL Oct 1, 2019
@trevor-scheer trevor-scheer merged commit 3a6b11a into master Oct 2, 2019
@abernix abernix deleted the abernix/avoid-checking-graphql-rules-by-Function.prototype.name branch October 2, 2019 11:05
@glasser
Copy link
Member

glasser commented Oct 3, 2019

This change broke the ability to use apollo in engine-frontend, because it seems like PossibleTypeExtensions was added in graphql-js 14.1 and we were still on 14.0.

I recognize that the peer dep from apollo-graphql has been 14.2.1 since June, but it looks like 14.0 worked before; I just want to make sure you're aware that the requirement is now more strict than it was before.

@trevor-scheer
Copy link
Member

@glasser thank you for pointing this out. Is this an issue that engine-frontend is currently dealing with, or were they able to upgrade / work around this?

So I understand correctly, is it accurate to say that engine-frontend has been ignoring an unmet peer dependency for some time based on their graphql version being too low?

@glasser
Copy link
Member

glasser commented Oct 7, 2019

I believe so.

trevor-scheer added a commit to apollographql/apollo-server that referenced this pull request Oct 7, 2019
This commit updates the apollo-graphql package to the latest
version. This includes a fix for a bug that currently breaks
minification. For reference, see:
apollographql/apollo-tooling#1551

Fixes #3335
abernix pushed a commit to apollographql/apollo-server that referenced this pull request Oct 8, 2019
This commit updates the apollo-graphql package to the latest
version. This includes a fix for a bug that currently breaks
minification. For reference, see:
apollographql/apollo-tooling#1551

Fixes #3335
abernix pushed a commit to apollographql/federation that referenced this pull request Sep 4, 2020
…3387)

This commit updates the apollo-graphql package to the latest
version. This includes a fix for a bug that currently breaks
minification. For reference, see:
apollographql/apollo-tooling#1551

Fixes apollographql/apollo-server#3335
Apollo-Orig-Commit-AS: apollographql/apollo-server@6a4b681
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buildFederatedSchema breaks when minified
3 participants