-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
deprecate use(Lazy)Query
on(Error|Completed)
callbacks
#12340
Conversation
🦋 Changeset detectedLatest commit: 5bea158 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
commit: |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give this another read-through in the morning, but here is some initial feedback. Hope this helps.
src/react/types/deprecation.md
Outdated
instead of pushing additional bundle size on all our users for a feature we | ||
don't recommend to use in the first place. | ||
|
||
### What to use instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be good to add one more section on "bad practices". You do mention that we don't recommend the use of onCompleted
right before this, but I think it would be good to go into a bit more detail on some specifics for why that is (e.g. cache can't be synced, etc.)
That, or perhaps state this early on in this document as one of the primary motivators behind this change so that much of this document can support that claim.
Basically I just want to make sure readers walk away with a solid understanding on why we don't recommend its use to begin with. Whether that is provided through its own section, or done as a statement in the beginning with the rest of this as supporting evidence would be great either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you clearly have a better idea what to write there than I do - could you maybe contribute that paragraph? 😅
src/react/types/deprecation.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Meschreiber would you perhaps be willing to take a look at this as well? We are deprecating some callbacks and want to provide some justification for doing so. Your feedback is always appreciated, especially since you're much better at English than I am, so your suggestions would probably make more sense than mine 😆
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phryneas made a couple other tweaks and added a section on "our recommendation". Feel free to tweak or revert any of my updates as you see fit, but otherwise looks great on my end 🎉
Once we settled on a wording of the "deprecation issue", I'll open an issue and add the link to that in the deprecation messages and the changeset.
I'll be very happy for any feedback on wording or arguments.