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: change OperationTypeNode to enum #2887

Closed
wants to merge 1 commit into from
Closed

fix: change OperationTypeNode to enum #2887

wants to merge 1 commit into from

Conversation

lekoaf
Copy link
Contributor

@lekoaf lekoaf commented Jan 20, 2021

Changes OperationTypeNode to enum instead of a union type. This allows developers to use the type as OperationTypeNode.QUERY instead of risking to msispell the string 'query'.

Example:

Instead of having to write

operation === 'query'

you can instead write

operation === OperationTypeNode.QUERY

This should not be a breaking change, since the string version should still work.

Changes OperationTypeNode to enum instead of a
union type. This allows developers to use the type
as `OperationTypeNode.QUERY` instead of risking to
msispell the string 'query'.

Example:

Instead of having to write

`operation === 'query'`

you can instead write

`operation === OperationTypeNode.QUERY`

This should not be a breaking change, since the string
version should still work.
@IvanGoncharov
Copy link
Member

@lekoaf Thanks for the PR 👍
We definitely need to merge it.
One thing that we currently in process of migrating our codebase to TS with the goal of minimal changes in mind, see #2828
Let's merge it after we switch master to TS.

@IvanGoncharov IvanGoncharov added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Jan 21, 2021
Base automatically changed from master to main January 27, 2021 11:10
Copy link
Member

@saihaj saihaj left a comment

Choose a reason for hiding this comment

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

Thanks @lekoaf! can you please rebase this. Then @IvanGoncharov we can merge this

@yaacovCR
Copy link
Contributor

Now that TS migration is done, this can perhaps be included in v16? Is there a reason this was held up?

@lekoaf
Copy link
Contributor Author

lekoaf commented Oct 10, 2021

Now that TS migration is done, this can perhaps be included in v16? Is there a reason this was held up?

Yupp, I missed the rebase comment. I'll do that tomorrow. 😃

@lekoaf
Copy link
Contributor Author

lekoaf commented Oct 12, 2021

I'm closing this because I didn't manage to rebase after all the changes. Most of all the base branch name. Please see #3312 instead.

@lekoaf lekoaf closed this Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants