-
Notifications
You must be signed in to change notification settings - Fork 2k
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
React 18 #77046
React 18 #77046
Conversation
This PR modifies the release build for wpcom-block-editor To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-l4k-p2 |
...t/editing-toolkit-plugin/wpcom-block-editor-nav-sidebar/src/components/create-page/index.tsx
Outdated
Show resolved
Hide resolved
Great work @noahtallen! 🚀 Thank you! I think we should first address the peer dep situation in #77121 which is quite complex on its own 😅 I'll devote some time to helping there as well with whatever I can. |
@tyxla I made a bunch of changes to the yarnrc file, including reworking how we ignore certain warnings. It should both be much more clear what our discard statements are doing, and should also dynamically cover more scenarios that are the same issue. I also documented the packages we're waiting for I also ignored the warnings related to the date picker package we decided not to update just yet. Finally, I also synced our react-virtualized fork (Automattic/react-virtualized#3) and published the update to npm, ultimately incorporating it here. This means all the violations/warnings happening on |
I added a bunch of little commits trying to get some insight into why yarn install is failing in CI and not locally. Here's what's happening. (TeamCity link) RUN cat packages/data-stores/package.json && yarn explain peer-requirements p031cd && yarn install && yarn explain peer-requirements p031cd && cat packages/data-stores/package.json
### packages/data-stores/package.json
{
"name": "@automattic/data-stores",
...
"peerDependencies": {
...
"react": "^17.0.2", ### WTF???
}
}
### First explain peer requirements (it's fine for some reason?)
YN0000: @automattic/domain-picker@workspace:packages/domain-picker [abd57] provides react-dom@npm:18.2.0 [35689] with version 18.2.0, which satisfies the following requirements:
YN0000: @automattic/data-stores@workspace:packages/data-stores [abd57] → ^18.2.0 ✓
### Yarn install output:
┌ Resolution step
│ @automattic/domain-picker@workspace:packages/domain-picker [abd57] provides react-dom (p031cd) with version 18.2.0, which doesnt satisfy what @automattic/data-stores requests
...
### Second explainer for same ID, now broken!!
@automattic/domain-picker@workspace:packages/domain-picker [abd57] provides react-dom@npm:18.2.0 [441bf] with version 18.2.0, which doesnt satisfy the following requirements:
@automattic/data-stores@workspace:packages/data-stores [abd57] → ^17.0.2 ✘
### packages/data-stores/package.json
{
"name": "@automattic/data-stores",
...
"peerDependencies": {
...
"react": "^17.0.2", ### incorrect!!!
}
} Some of these symptoms are very similar to p9oQ9f-1I5-p2. This is so bizarre |
Glad to hear - nice work!
For what it's worth I've tried on both my machines to repro it and could not. I wonder if we have some sort of a cache on the TeamCity level that we need to bust 🤔 |
Yeah, if I check the contents of a package.json file outside of the docker build, it is correct and matches the branch. But inside the build, after running |
Ok, the issue is not happening any more. I rebased the PR and squashed a ton of commits, and the weird CI issue with peer dependency warnings is resolved. |
This PR modifies the release build for notifications To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-elI-p2 |
I did further testing of the Calypso apps like ETK, notifications, and wpcom-block-editor, and these are also working well. |
Next up, updating WordPress packages to latest: #78711 |
* Update packages to be compatible with React 18 * Update internal peer dependencies * Resolve peer dependency issues * Update several internal tsconfig references * Add stream fallback in webpack * Attempt removing jsx import alias * Update snapshots to remove whitespace * Remove custom @types/react patch and pin react 18 types * Fix issues with children type in React props * Fix type issues related to useCallback function arguments * Fix issues with the store selector types * Remove some uneeded React types * remove an unused ts-expect-error call * Add ReactNode types to an inline function component * Change a use of a component to call it directly * Fix a type related to interpolate element (ReactNode->ReactElement) * Cast known render values to string * Fix context types in legacy class component * Fix incorrect prop type (Node->ReactNode) * Fix a type issue with an action creator * Remove fallback translation which was causing a type error * Fix an odd issue where children were passed as a function * Improve generics for use track callback useCallback * Cast render type to ReactNode and add comment * Cast const array to readonlyarray so we can use .includes on it * Another yarn.lock update * Enable allowDeclareFields for typescript classes in babel preset * Fix babel config preset syntax * Try with casting for now * Fix a bunch of package unit tests * Fix a bunch of client unit tests * Conditionally run layout effects in SSR * Import react dom server from browser bundle explicitly * Attempt defining TextEncoder globally for tests * Attempt downgrading use-subscription to fix SSR bug --------- Co-authored-by: Marin Atanasov <[email protected]>
Translation for this Pull Request has now been finished. |
This PR updates Calypso to React 18, and updates WordPress packages to the first version which supported React 18 (which is 4-5 months old at this point.)
Note: this does not yet enable the new rendering mode -- that will likely be a follow-up due to the size of this PR!
Prerequisite or split-out work:
Notable breaking changes which impact us (excluding concurrent mode)
useCallback
now requires types for the callback arguments, which means most of those need to be updatedchildren
isn't automatically a prop, so we need to add it, which means everywhere we use children needs to be updatedTo do:
Done: (🤞 )
useSyncExternalStore
being called without a 3rd argument inuse-subscription
. (This breaks SSR.)Importing(Will potentially need to fix this in a future WP package update)parseWithAttributeSchema
from@wordpress/blocks
crashes the application -- something related to the unlock/lock private APIs. This unfortunately prevents the editor routes from loading.declare context
use (breaking docker build)createElement
:Argument of type 'ForwardRefExoticComponent<ButtonProps & RefAttributes<any>>' is not assignable to parameter of type 'string | FunctionComponent<BaseButtonProps & _ButtonProps & Omit<Omit<DetailedHTMLProps<ButtonHTMLAttributes<HTMLButtonElement>, HTMLButtonElement>, "ref">, "disabled" | ... 1 more ... | keyof BaseButtonProps> & RefAttributes<...>> | ComponentClass<...>'.
(example) -- doesn't seem to happen any moreExternalLink
anduseInterpolateElement
(punted)stream
browserify to calypso-build and client@types/react
has changed to be more strict about this.ComponentProps< typeof Component >
now that prop types aren't directly available for wp components.stream
package -- apparently this was needed for react-dom? Webpack wouldn't compile without it.isX
. (E.g.variant="tertiary"
rather thanisTertiary
)@types/
packages required by@wordpress/components
(See Add some @types packages as proper dependencies WordPress/gutenberg#50231 for more info, but we'll need to do this in Calypso until we get a fix in core.)@automattic/global-styles
-- it relies extensively on private file imports (e.g.@wordpress/edit-site/build-module/some/deep/path
), which have been drastically changed internally.