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 duplicate queries in named exports #170

Merged
merged 2 commits into from
Apr 19, 2018

Conversation

wacii
Copy link
Contributor

@wacii wacii commented Apr 18, 2018

Recent changes to the loader exported queries as individual named exports, but each such export erroneously included the previous exports. This gets big fast.

Closes #168

Recent changes to the loader exported queries as individual named
exports, but each such export erroneously included the previous exports.
loader.js Outdated
// Make copy before named exports are added to the doc
// see https://github.com/apollographql/graphql-tag/issues/168
outputCode += `
var docCopy = Object.assign({}, doc);
Copy link
Contributor

@jnwng jnwng Apr 18, 2018

Choose a reason for hiding this comment

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

this is partially my fault for not establishing what our compatibility matrix is, but this almost certain will break for ie 11. is it possible to use an alternative method here to assign / clone this object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. The only properties on GraphQL AST's are kind, definitions, and loc, right? Would you like me to change the other instances of Object.assign() while I'm at it?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, yes, i didnt notice that we had those in there as well. if its not too much trouble, that would be great! otherwise im happy to merge this and we can address the compatibility issue afterwards (since its already an issue)

@jnwng
Copy link
Contributor

jnwng commented Apr 18, 2018

thanks for the pr @wacii, certainly appreciated. just one question but otherwise looks fine to proceed

@Amareis
Copy link

Amareis commented Apr 19, 2018

Merge that, merge it, merge now. It's really bad bug!

@jnwng jnwng merged commit 84010ee into apollographql:master Apr 19, 2018
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.

3 participants