Skip to content

Remove 'devAssert' checks that duplicate TS types #3642

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 13, 2022
Merged

Remove 'devAssert' checks that duplicate TS types #3642

merged 1 commit into from
Jun 13, 2022

Conversation

IvanGoncharov
Copy link
Member

graphql provides TS types since 14.5.0 (released 3 years ago)
and we fully switched to TS in 15.0.0 so I think it's time to drop
runtime typechecks.

Motivations: This type checks were added long time ago since we shifted
towards TS we just maintained them without adding new ones.
In general, this check increase bundle size add runtime cost and we
can't realistically check all arguments to all functions.
Instead we should focus on adding more asserts on stuff that can't be
checked by TS.

@IvanGoncharov IvanGoncharov added the PR: breaking change 💥 implementation requires increase of "major" version number label Jun 13, 2022
@IvanGoncharov IvanGoncharov requested a review from yaacovCR June 13, 2022 15:54
@netlify
Copy link

netlify bot commented Jun 13, 2022 •
edited
Loading

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 9318a8d
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62a7a49226538e00095466a2
😎 Deploy Preview https://deploy-preview-3642--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.

@github-actions
Copy link

Hi @IvanGoncharov, 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

@yaacovCR
Copy link
Contributor

Not that I am against — but I believe the reason devAssertions have been retained is to help out those coding in JS without access to IDE help from TS. I don’t believe we ”forgot“ to remove them. We have chosen until now to keep supporting JS at this level.

I do agree, however, that in terms of our cost/benefit analysis, we should remove them — in favor of more generic help to JavaScript developers via the exported types. I hope our JavaScript users agree!

@IvanGoncharov
Copy link
Member Author

I don’t believe we ”forgot“ to remove them. We have chosen until now to keep supporting JS at this level.

Yes, you're right it wasn't "forgotten" moreover I maintained them and migrated from generic invariant to more specific devAssert some time ago. But I think as time passes they become less useful for the ecosystem, so it's time to drop them.
That said we still have some of the runtime checks to enforce stuff not supported by the TS type system, e.g. exact types.

@IvanGoncharov
Remove 'devAssert' checks that duplicate TS types
9318a8d 
`graphql` provides TS types since `14.5.0` (released 3 years ago)
and we fully switched to TS in `15.0.0` so I think it's time to drop
runtime typechecks.

Motivations: This type checks were added long time ago since we shifted
towards TS we just maintained them without adding new ones.
In general, this check increase bundle size add runtime cost and we
can't realistically check all arguments to all functions.
Instead we should focus on adding more asserts on stuff that can't be
checked by TS.
@IvanGoncharov IvanGoncharov merged commit a4b085b into graphql:main Jun 13, 2022
@IvanGoncharov IvanGoncharov deleted the pr_branch2 branch June 13, 2022 21:01
@IvanGoncharov IvanGoncharov mentioned this pull request Jun 13, 2022
Merged
@yaacovCR yaacovCR mentioned this pull request Sep 7, 2022
Closed
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Jan 1, 2023
@yaacovCR
remove devAsserts that duplicate TS types
61d6f8c 
see graphql#3642
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Apr 3, 2023
@IvanGoncharov
GraphQLInputObjectType: remove check that duplicate TS types
99244bc 
Context: Continuation of graphql#3642
@IvanGoncharov IvanGoncharov mentioned this pull request Apr 3, 2023
Merged
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Apr 4, 2023
@IvanGoncharov
GraphQLInputObjectType: remove check that duplicate TS types
478781d 
Context: Continuation of graphql#3642
@jasonkuhrt jasonkuhrt mentioned this pull request Oct 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaacovCR yaacovCR Awaiting requested review from yaacovCR

Assignees
No one assigned
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@IvanGoncharov @yaacovCR