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

let TVariable generics extend OperationVariables #1910

Merged
merged 3 commits into from
Feb 10, 2023

Conversation

phryneas
Copy link
Contributor

@phryneas phryneas commented Feb 8, 2023

to accommodate an upstream type change in @apollo/[email protected]

This addresses #1907

Checklist:

  • If this PR is a new feature, please reference a discussion where a consensus about the design was reached (not necessary for small changes)
  • Make sure all the significant new logic is covered by tests
  • Try to include the Pull Request inside of CHANGELOG.md

@changeset-bot
Copy link

changeset-bot bot commented Feb 8, 2023

🦋 Changeset detected

Latest commit: ad5340f

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

This PR includes changesets to release 1 package
Name Type
apollo-angular Patch

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

Copy link
Collaborator

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

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

According to CI, you might have forgotten a few usages...

package.json Outdated
@@ -35,7 +35,7 @@
"@angular/platform-browser-dynamic": "^14.0.6",
"@angular/platform-server": "^14.0.6",
"@angular/router": "^14.0.6",
"@apollo/client": "3.7.5",
"@apollo/client": "3.7.7",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This definitely should use ^ notation, and probably be ^3.7.7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I just wanted to stick to existing patterns :)

@@ -0,0 +1,5 @@
---
'apollo-angular': minor
Copy link
Collaborator

Choose a reason for hiding this comment

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

In an ideal world apollo-client would have released a major version, because it was breaking, and we would also have to release a major version. But since they released as patch version, we probably also should release as patch, not minor, shouldn't we ?

@phryneas
Copy link
Contributor Author

phryneas commented Feb 8, 2023

Oh, sorry! I had only run tsc in the package folder and assumed that would be enough to catch everything.

I already finished up for the day - I'll get back to this and fix up the rest tomorrow.

@phryneas
Copy link
Contributor Author

phryneas commented Feb 9, 2023

Okay, next turn in the CI :)

Generally, these are hard to catch, since they don't turn up in the repo itself (which has strict: false), but only if used in other projects (as the CI does) with a different tsconfig and skipLibChecks: false.
Let's hope I did get all usages now.

I didn't turn on strict: true for the repo since that results in a big batch of unrelated strictNullCheck-caused errors, but personally I would really want to encourage you to make that change in the future, since this might reoccur - consuming projects with a more strict tsconfig and skipLibChecks: false might run into issues with this. (Also, strict: true is how the TS team intends TS to be used)

@PowerKiKi
Copy link
Collaborator

100% agree we must switch to strict typescript as soon as possible. I was not aware we were not strict yet.

@phryneas
Copy link
Contributor Author

phryneas commented Feb 9, 2023

Looks like it's al green now :) Yay!

@PowerKiKi PowerKiKi merged commit ff0b0d7 into kamilkisiela:master Feb 10, 2023
@PowerKiKi
Copy link
Collaborator

Thank you taking the time to fix 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