Skip to content

extendSchema: Do not modify standard directives#3618

Merged
IvanGoncharov merged 1 commit intographql:mainfrom
IvanGoncharov:pr_branch2
Jun 3, 2022
Merged

extendSchema: Do not modify standard directives#3618
IvanGoncharov merged 1 commit intographql:mainfrom
IvanGoncharov:pr_branch2

Conversation

@IvanGoncharov
Copy link
Copy Markdown
Member

Without this change:

const extendedSchema = extendSchema(schema, parse(extensionSDL));
extendSchema.getDirective('skip') === GraphQLSkipDirective
// false

Maybe in future we need more comprehensive solution, but for now it's enouch to just match behaviour that we already have to standard types.

@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label May 30, 2022
@netlify
Copy link
Copy Markdown

netlify Bot commented May 30, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 446b3c3
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/629a8805715f3200091c2f5d
😎 Deploy Preview https://deploy-preview-3618--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yaacovCR
Copy link
Copy Markdown
Contributor

CI broken :(

Change looks ok but somewhat unnecessary as recreated specified directive objects are identical to old directives, only necessary if for example schema modifiers are checking for specified directives via object identity rather than name, which should probably be avoided, as this "mistake" might be made by others.

For example. mapSchema from graphql tools recreates specified directives...

@IvanGoncharov
Copy link
Copy Markdown
Member Author

CI broken :(

Fixed, you can now rerun CI on your PRs.

@IvanGoncharov
Copy link
Copy Markdown
Member Author

Change looks ok but somewhat unnecessary as recreated specified directive objects are identical to old directives, only necessary if for example schema modifiers are checking for specified directives via object identity rather than name, which should probably be avoided, as this "mistake" might be made by others.

Yes, but it would be a global, even in our own code:
https://github.com/graphql/graphql-js/blob/main/src/utilities/astFromValue.ts#L128

@IvanGoncharov
Copy link
Copy Markdown
Member Author

CI broken :(

Fixed, you can now rerun CI on your PRs.

Sadly, GitHub UI doesn't allow to rerun it.
So I just force pushed to trigger it.

@yaacovCR
Copy link
Copy Markdown
Contributor

Uncovered problems with #3611 ?

@yaacovCR
Copy link
Copy Markdown
Contributor

yaacovCR commented Jun 2, 2022

CI working again -- if you want me to rebase on main and merge, just let me know.

Without this change:
```
const extendedSchema = extendSchema(schema, parse(extensionSDL));
extendSchema.getDirective('skip') === GraphQLSkipDirective
// false
```

Maybe in future we need more comprehensive solution, but for now it's enouch to just match behaviour that we already have to standard types.
@IvanGoncharov IvanGoncharov merged commit fdc3555 into graphql:main Jun 3, 2022
@IvanGoncharov IvanGoncharov deleted the pr_branch2 branch June 3, 2022 22:21
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 16, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 17, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 17, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 22, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: bug fix 🐞 requires increase of "patch" version number

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants