-
Notifications
You must be signed in to change notification settings - Fork 843
Protect Components: Use @wordpress/dataviews/wp import path #41510
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
Protect Components: Use @wordpress/dataviews/wp import path #41510
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 🔴 Action required: Please add missing changelog entries for the following projects: Use the Jetpack CLI tool to generate changelog entries by running the following command: Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCannot generate coverage summary while tests are failing. 🤐 Please fix the tests, or re-run the Code coverage job if it was something being flaky. |
8cc7233 to
65b7cd0
Compare
|
Build currently fails with the following: In GitHub build (logs): When running locally ( |
33306a4 to
dd2f628
Compare
31495ea to
9da5046
Compare
65e404a to
907c3b8
Compare
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.
You've got a lot of accidental downgrades going on here. You should probably do something like git fetch && git checkout $( git merge-base HEAD origin/add/protect/core ) pnpm-lock.yaml && pnpm dedupe to regnerate this file.
1ad1cae to
4386869
Compare
dbeb4ff to
672faeb
Compare
|
This PR has been updated. Summary of changes:
Rebased and rebuilt pnpm to address unintentional downgrades. Currently looking into why the JS Tests are failing: The above error is introduced when switching between importing from |
ed169af to
18acf38
Compare
672faeb to
45cf11f
Compare
d8c8932 to
fb4fcf1
Compare
d9c496f to
c97a1b2
Compare
|
The test failure looks like the same thing as described in the "ESM" section of WordPress/gutenberg#67897. TL;DR: The way |
| // Missing deps. | ||
| // https://github.com/WordPress/gutenberg/issues/67864 | ||
| if ( pkg.name === '@wordpress/dataviews' ) { | ||
| for ( const dep of [ |
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.
How did you compile this list? Was it by looking at the errors during building?
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 don't remember how I got it for WordPress/gutenberg#67864. 😀 Possibly that, or possibly by grepping build-wp/index.js for import statements and then removing the ones that are already dependencies of @wordpress/dataviews.
| { | ||
| textdomain: 'jetpack-protect', | ||
| functions: { | ||
| __: 1, |
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.
Why do we need the functions object here?
Was the compiling manual? If yes, would that mean updating things here if the core DataViews package adds more i18n functions in its code?
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 real problem is that build-wp/index.js isn't preserving the names of the @wordpress/i18n functions. Instead it does a lot of aliasing like import { __ as __9 } from '@wordpress/i18n';.
It turns out that Webpack as used in this particular PR currently winds up undoing all the bad import aliasing, going back to the correct __ name so GlotPress's use of wp i18n will pick up the strings from the bundle. But @automattic/babel-plugin-replace-textdomain (which runs long before Webpack fixes it) needs this to know to inject the correct textdomain into all the aliased calls, because the translations won't be in Core's default domain.
Someday, when Core distributes @wordpress/dataviews like it does @wordpress/components and other packages, none of this will be needed because both the script and translations will come from Core instead of having to be bundled with the plugin. It's unfortunate that we're currently in this situation where @wordpress/dataviews is too unstable for Core to provide but everyone wants to start using it anyway.
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.
A beautiful thing to see this working in Protect now without Gutenberg - very nicely done!
That said, I wasn't able to get the Storybooks to load with various @wordpress+dataviews webpack specific build errors running the provided command. Perhaps I need to reinstall or clear/rebuild something but each of the combinations I tried that have worked previously didn't seem to help or caused other issues. Is it working for you? If so, any recommendations on how to get that configured properly?
There's one error for '@wordpress/hooks' and another for '@wordpress/date' - example:
ERROR in ../../../node_modules/.pnpm/@[email protected]_patch_hash=9971bdb899e2fb0a4421ab8b7f0a54a41f03b10ef1b51d72_b9a36bf3010841a36adc99ad3275ff5d/node_modules/@wordpress/dataviews/build-wp/index.js 422:0-44
Module not found: Error: Can't resolve '@wordpress/hooks' in '/Users/dkmyta/dev/jetpack/node_modules/.pnpm/@[email protected]_patch_hash=9971bdb899e2fb0a4421ab8b7f0a54a41f03b10ef1b51d72_b9a36bf3010841a36adc99ad3275ff5d/node_modules/@wordpress/dataviews/build-wp'
|
Sigh. I guess I'd probably include a comment to point out the difference between these and the rest. |
|
Storybooks are working nicely with the wordpress deps added 👍 |
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.
LGTM!
The same requests/nit-picks I've outlined here apply, but I think we can go ahead and merge this into the project branch and deal with those in trunk.
UPDATE: I've approved, but there are some outstanding failing tests.
5ec6013 to
9499f09
Compare
cbcf1bc to
b9cad5f
Compare
9499f09 to
bcbb401
Compare
b9cad5f to
a104f99
Compare
bcbb401 to
ae2effe
Compare
a104f99 to
a8761cf
Compare
This reverts commit e4f9321.
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 change to using the @wordpress/dataviews/wp import path appears to break tests that use the dataviews package... removed this basic render test for now.
See WordPress/gutenberg#66825 for more info on the
@wordpress/dataviews/wpimport path.Proposed changes:
import ... from '@wordpress/dataviews'toimport ... from '@wordpress/dataviews/wp'.Other information:
Jetpack product discussion
peb6dq-3ta-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
ThreatsDataViewsandScanReportcomponents.cd projects/js-packages/storybook && pnpm storybook:dev