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

First draft for upgrade guide to v17 #4310

Draft
wants to merge 1 commit into
base: 16.x.x
Choose a base branch
from
Draft

Conversation

JoviDeCroock
Copy link
Member

No description provided.

Copy link

Hi @JoviDeCroock, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM


## Removals

- Removed exported `getOperationType` function
Copy link
Contributor

@yaacovCR yaacovCR Jan 30, 2025

Choose a reason for hiding this comment

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

Suggested change
- Removed exported `getOperationType` function
- Removed deprecated `getOperationType` function, use instead the `getRootType()` method on the
appropriate `GraphQLSchema` object.

## Removals

- Removed exported `getOperationType` function
- Removed exported `getVisitFn` function
Copy link
Contributor

@yaacovCR yaacovCR Jan 30, 2025

Choose a reason for hiding this comment

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

Suggested change
- Removed exported `getVisitFn` function
- Removed deprecated `getVisitFn` function, use `getEnterLeaveForKind` instead.


- Removed exported `getOperationType` function
- Removed exported `getVisitFn` function
- Removed exported `printError` and `formatError` utilities
Copy link
Contributor

@yaacovCR yaacovCR Jan 30, 2025

Choose a reason for hiding this comment

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

Suggested change
- Removed exported `printError` and `formatError` utilities
- Removed deprecated `printError` and `formatError` utilities, use instead the `toString` and `toJSON` methods on the desired `GraphQLError` object.

- Removed exported `getOperationType` function
- Removed exported `getVisitFn` function
- Removed exported `printError` and `formatError` utilities
- Removed exported `assertValidName` and `isValidNameError` utilities
Copy link
Contributor

@yaacovCR yaacovCR Jan 30, 2025

Choose a reason for hiding this comment

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

Suggested change
- Removed exported `assertValidName` and `isValidNameError` utilities
- Removed deprecated `assertValidName` and `isValidNameError` utilities, convert to `assertName`.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have a standard of doing empty braces? Feels a bit redundant

Copy link
Contributor

Choose a reason for hiding this comment

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

removed, to each their own!

- Removed exported `printError` and `formatError` utilities
- Removed exported `assertValidName` and `isValidNameError` utilities
- Removed exported `assertValidExecutionArguments` function
- Removed `getFieldDefFn` from `TypeInfo`
Copy link
Contributor

@yaacovCR yaacovCR Jan 30, 2025

Choose a reason for hiding this comment

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

Suggested change
- Removed `getFieldDefFn` from `TypeInfo`
- Removed deprecated `getFieldDefFn` method from `TypeInfo`. If you really need to customize field resolution, you can subclass the `GraphQLSchema` class and override the `getField` method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would heavily avoid subjective strengtheners like truly

Copy link
Contributor

Choose a reason for hiding this comment

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

I switched it to the "if you really need this" construction used elsewhere

- Removed exported `assertValidName` and `isValidNameError` utilities
- Removed exported `assertValidExecutionArguments` function
- Removed `getFieldDefFn` from `TypeInfo`
- Removed `TypeInfo` from `validate` https://github.com/graphql/graphql-js/pull/4187
Copy link
Contributor

@yaacovCR yaacovCR Jan 30, 2025

Choose a reason for hiding this comment

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

Suggested change
- Removed `TypeInfo` from `validate` https://github.com/graphql/graphql-js/pull/4187
- Removed deprecated `TypeInfo` argument from `validate`, this was previously used to support non-specified validation behavior that is explicitly discouraged; custom behavior can be better obtained by using custom rules.

I removed the reference to the PR which has no additional info, and we didn't link to PRs generally, but it's fine really to keep.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, the PR's having no context is its own issue. You're right though, none of the PR's give the context we require to pick all of this up. Not sure whether we need to specify whether it was deprecated though, I don't think that matters at all

Copy link
Contributor

Choose a reason for hiding this comment

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

The advantage of sprinkling the word deprecating in is that it hopefully gives the reader the (correct) understanding that although the list of breaking changes may be lengthy, it is shorter if they are not using deprecated functionality. This might be immediately helpful to those who know that they are not using deprecated functionality and might encourage those still using deprecated functionality to migrate off of it if they have not done so. I have changed elsewhere exported to deprecated as a one word change that gives this impression as a whole. Here I am adding the one word for consistency.

In terms of the no context on the linked PR, that is certainly my bad as it is mine, I take that constructive criticism, will try to improve in the future. I have edited suggestion above to give more context inline.

- Removed exported `getVisitFn` function
- Removed exported `printError` and `formatError` utilities
- Removed exported `assertValidName` and `isValidNameError` utilities
- Removed exported `assertValidExecutionArguments` function
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Removed exported `assertValidExecutionArguments` function
- Removed deprecated function `assertValidExecutionArguments()` function, use `assertValidSchema()` to validate the schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

We haven't actually deprecated this in v16 I think, I will submit a PR to do this. All the additional checks in that function are now just performed by TS.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yaacovCR
Copy link
Contributor

@JoviDeCroock I have started working on merging in what I have been tracking in #4198 ... these are a few suggestions so far, let me know if you'd rather have me open up PRs to this PR rather than do it like this.

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.

2 participants