-
Notifications
You must be signed in to change notification settings - Fork 198
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
Update npm dependencies (especially @wordpress
)
#7695
base: trunk
Are you sure you want to change the base?
Conversation
427e4f8
to
41fe999
Compare
41fe999
to
710033b
Compare
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
WordPress Dependencies ReportThe
This comment was automatically generated by the |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
3e6bfe9
to
6ca3cf5
Compare
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
@@ -119,6 +118,8 @@ | |||
"react": "17.0.2", | |||
"react-dom": "17.0.2", | |||
"resolve-url-loader": "5.0.0", | |||
"sass": "1.35.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.
The @wordpress/scripts
installs other minor versions (it uses the ^
), and sass introduced a lot of deprecations because its API is changing.
I checked that Gutenberg codebase is still using it in the old way, so I also kept it using the same approach by installing the specific version.
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.
Before checking how Gutenberg was doing it, I tried to fix the deprecations but it's risky to break something and it's a big amount of work. So it's better to wait for Gutenberg decisions before changing it in Sensei. Notice that we also import scss files directly from Gutenberg and Calypso (which use the deprecated @import
).
@@ -63,7 +63,7 @@ const NoticeActions = ( { actions } ) => { | |||
return ( | |||
<div className="sensei-notice__actions"> | |||
{ actions.map( ( action ) => ( | |||
<NoticeAction key={ action.url } action={ action } /> | |||
<NoticeAction key={ action.label } action={ action } /> |
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.
Make sure the key will be always available since url
is not always available.
issuer: styleSheetFiles, | ||
type: 'asset/resource', |
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.
By default @wordpress/scripts
is adding it as type: 'asset/inline',
for style files.
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.
@renatho could you help me out with this? I've managed to break it when updating the packages and can't figure out why the inline SVGs are causing this error:
ERROR in ../node_modules/@automattic/data-stores/node_modules/@automattic/calypso-products/node_modules/@automattic/components/src/icons/protect.tsx 4:1
Module parse failed: Unexpected token (4:1)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
|
| const icon = (
> <SVG width="16" height="20" viewBox="0 0 16 20" fill="none" xmlns="http://www.w3.org/2000/svg">
...
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.
Hi @m1r0! I'll take a look and see if I can find the cause on Monday. 😉
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've managed to solve the above by updating @automattic/components
and @automattic/tour-kit
(related PRs Automattic/wp-calypso#90267 woocommerce/woocommerce#47129) but now the test suite is throwing Cannot unlock an object that was not locked before
. 🙄 Are you familiar with this error?
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 found that this is an error coming from Gutenberg: WordPress/gutenberg@1b18104#diff-a115d786e2db4593eaf8ecdf7126885b1fb7f2c936c2884de7f52a5ab00eb1e0R142-R144
But no idea of what it means without investigating deeper. :(
Test the previous changes of this PR with WordPress Playground. |
PR hand-offIf we don't have enough time to work complete this and we want to release it partially, we could reset to this commit: 21c0b6f, and do some tests in the site to make sure everything is working well from a user perspective. In this commit: 21c0b6f, I started some more "aggressive" changes, and they are not complete. With that, we need to continue fixing other things. The next steps: JS unit testsThe commit d4d1f7e is a "draft". I sent it as a reference for ideas to solve the existing issues with tests. Later when we have the solution better defined we can amend to this commit or even remove it. See this relevant PR related to this: WordPress/gutenberg#47388 I was getting ideas from Gutenberg repository, and I think there we can still find all answers we need, just exploring more. Based on this comment, I thought it should be simpler, but I wasn't able to make it work yet, even in a fresh environment that I tried. StylesWe import Although it shouldn't impact the next steps of this PR, see #7695 (comment) for more relevant things about styles. E2E testsSome related dependencies were already updated, but I didn't look at it yet. LintI also didn't look at lint either. I suspect this one won't be very complex. Final thingsAs described in the issue, make sure Sensei Pro continues working properly before it's merged. |
@wordpress
)
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
ea247dc
to
cda2399
Compare
Test the previous changes of this PR with WordPress Playground. |
cda2399
to
ab5f7be
Compare
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Test the previous changes of this PR with WordPress Playground. |
Resolves #7692 #6123
Proposed Changes
Testing Instructions
New/Updated Hooks
Deprecated Code
Pre-Merge Checklist