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

Remove variables only used by internal directives #310

Conversation

alexlafroscia
Copy link
Contributor

This PR removes the variables only used by internal directives. See the related issue for more details about why this is important.

Closes #308

@changeset-bot
Copy link

changeset-bot bot commented Jun 3, 2022

🦋 Changeset detected

Latest commit: 840803c

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@alexlafroscia alexlafroscia force-pushed the remove-local-directives-from-queries branch from f5d9715 to 4a4eb0e Compare June 3, 2022 17:07
Copy link
Collaborator

@AlecAivazis AlecAivazis left a comment

Choose a reason for hiding this comment

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

Thanks for getting this out so quickly!

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Jun 4, 2022

@AlecAivazis is there a reason this project is using such an old version of TypeScript? I am running into a problem with the code I am refactoring to, where the TypeScript version provided by my editor's language tools are OK with the code (since it defaults to TypeScript 4.6) but the tests are failing because the version used by the project (3.9.5) doesn't seem to like it.

On 4.6 the instanceof check narrows the type correctly

CleanShot 2022-06-04 at 10 02 18@2x

On 3.9.5 it doesn't

CleanShot 2022-06-04 at 10 02 32@2x

Unfortunately I can't use Array.isArray here due to microsoft/TypeScript#17002 (I think; for some reason Array.isArray doesn't narrow the type to remove the possibility that parent is an Array, and since the type from graphql is possibly a readonly Array, that seems likely to be the reason why)

@AlecAivazis
Copy link
Collaborator

Like I mentioned in the other PR, I am more than happy to update the typescript version if we can get the build to pass. Feel free to update it here instead of a separate PR if you prefer that way you don't have to juggle too many things going in at once

@alexlafroscia
Copy link
Contributor Author

@AlecAivazis gotcha! Juggling a bunch of branches doesn't matter to me; keeping things separate is fine by me.

In case I can't figure out why the TypeScript version bump breaks CI, feel free to merge this PR without the TypeScript update if that's what you want to do! I would love to get a Houdini version published with this bug fix in it so I can fix my app at work 😬

@alexlafroscia
Copy link
Contributor Author

Since I was able to get #312 passing, if you're happy with both PRs, let's merge that one first; I'll then rebase this one on main again to I can remove the silly @ts-expect-error comment that's currently necessary for this code to work correctly.

@AlecAivazis
Copy link
Collaborator

AlecAivazis commented Jun 5, 2022

@alexlafroscia its merged now! This should be good to go once you rebase and we get the 👍 from github

@alexlafroscia alexlafroscia force-pushed the remove-local-directives-from-queries branch from 2c0f33b to 840803c Compare June 6, 2022 14:03
@alexlafroscia
Copy link
Contributor Author

@AlecAivazis I rebased the PR on main -- looks like we're all set here!

Copy link
Collaborator

@AlecAivazis AlecAivazis left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for adding a test 👍

@AlecAivazis AlecAivazis merged commit 5cba9e2 into HoudiniGraphql:main Jun 6, 2022
@github-actions github-actions bot mentioned this pull request Jun 6, 2022
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.

Variables used only by directives should be removed from the request
2 participants