-
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
Remove the deprecated hoc and query components #12211
Remove the deprecated hoc and query components #12211
Conversation
✅ Docs Preview ReadyNo new or changed pages found. |
🦋 Changeset detectedLatest commit: 3898c13 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 📦
|
"optimism": "^0.18.0", | ||
"prop-types": "^15.7.2", |
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 love to see these go.
import { graphql } from "../../graphql"; | ||
import { ChildProps, DataValue } from "../../types"; | ||
|
||
describe("SSR", () => { |
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 believe that we don't have any of these tests for the hooks, so maybe we should migrate the ssr tests over first?
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.
Good call! I added src/react/ssr/__tests__/getDataFromTree.test.tsx
and migrated these tests to use hooks. Should be good to go!
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.
While I'm very much in favor of these changes, I think we need to go through all deleted tests and make sure we have a similar tests for the hooks, or port that test over, before deleting them.
|
||
if (React.version.startsWith("19")) { | ||
// react-dom/server uses MessageChannel in React 19 | ||
window.MessageChannel = jest.fn().mockImplementation(() => { |
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.
Unfortunately JSDOM doesn't have support for MessageChannel
which it appears React is using with React 19 and renderToStaticMarkup
.
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.
4694d20
to
a3f8658
Compare
Still staying with "changes requested" for now, though - a lot of the HOC/renderProps tests were implicitly testing the hooks, and we really need to double-check that we don't have any functionality that was only tested in these and not the hooks tests. |
I've looked back through all the deleted tests and I can't find anything that we haven't accounted for already. Most of the tests are checking details about the HOC/render props APIs. We've got a much more robust test suite for our existing hooks than what was currently there. Besides, we've been rolling with our |
Verbally approved :) |
Closes #12189
Removes the
Query
,Mutation
,Subscription
,withQuery
,withMutation
,withSubscription
,withApollo
, andgraphql
components. As such, we have also been able to remove the dependency onhoist-non-react-statics
and moveprop-types
to a dev dependency (it looks like some of our docs components use this)