-
Notifications
You must be signed in to change notification settings - Fork 800
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 webpack globally #20789
Update webpack globally #20789
Conversation
Caution: This PR has changes that must be merged to WordPress.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.
Here are some suggested test cases for this PR.
Connection
- In-place connection with free plan
- In-place connection with paid plan
- In-place connection with product purchase
- Classic connection. Use Safari, or set a constant
JETPACK_SHOULD_NOT_USE_CONNECTION_IFRAME
to true - Disconnect/reconnect connection
- Secondary user connection
- Connection on multisite
Verify that the changes are compatible with the plugins that use the connection package.
- WooCommerce Payments
- Jetpack Boost
- Previous versions of Jetpack
If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.
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 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
Backup plugin:
|
sorry I was taking a look at the PR and changed the title accidentally :-( |
It added a @todo saying to make it use calypso-build again, which I figured out how to do.
Webpack 5 breaks if you pass `argv.entry` through as `entry` from your config function. Webpack 4 was ok with that. I have no idea whether that's a bug in webpack 5 for breaking that, or calypso-build for passing it through instead of letting webpack handle it. Either way, this works around it. Also of note is that calypso-build now declares webpack as a peer dep, so lazy-images needs to depend on it to provide the peer.
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.
Looks sensible to me. I fixed lazy-images to continue using calypso-build, see a72fa61 for an explanation of what was breaking there.
@anomiex thanks for the patch. Unfortunately it broke es5 validation for the built lazy-images package.
I'll jump in and fix this, since I already fixed it previously. |
No more IE11 => newer ecmaVersion
Oops, I already did before I saw your comment. |
…etpack into try/update-webpack-globally
@anomiex I reverted the change to es9 validation since it emits code that won't run in some browsers. Better to just emit es2015 as before, just in case. If we want to make a change to which browsers we support it should probably happen in a different PR. |
We already did drop support for IE11. p1HpG7-c8n-p2 |
If it's cool with everyone else to update browser compat for this library in this PR then I'm fine with that. I just didn't want to introduce accidental/unexpected breakage outside the scope of the change. But if it's not going to break anything then let's go es9! |
Actually... I don't have any idea what calypso-build is doing, but just adding the
bit (and the dep on I have no idea what's going on with that, since |
Good catch. I'm not at my computer right now so if you want to revert my
change and add browserlist back in go right ahead, otherwise I'll take a
look later or tomorrow.
…On Mon, Aug 23, 2021, 2:06 PM Brad Jorsch ***@***.***> wrote:
Actually... I don't have any idea what calypso-build is doing, but just
adding the
"browserslist": [
"extends @wordpress/browserslist-config"
],
bit (and the dep on @wordpress/browserslist-config) is somehow enough to
restore IE11 support when I build locally. All the other stuff you added in
9b17538
<9b17538>
seems unnecessary.
I have no idea what's going on with that, since
@wordpress/browserslist-config 4.1.0 doesn't include IE11 so if anything
I'd think it should be the opposite.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#20789 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAMVOGHIUTIE6NFERKMXSTT6KZ4PANCNFSM5CSNP7UQ>
.
|
OK, I see what's going on now.
Thus the output winds up compatible with ES5, so the validate-es5 check passes. But the "op_mini all" is apparently ignored by Babel, so if we were to update the source files (which we probably should do one of these days now that we use calypso-build) the validate-es5 check would no longer pass. What we really need to do is to figure out how to make a validate check that uses the browserslist targets instead of just guessing at an appropriate ES parser level. See p9dueE-3fG-p2 for more on that topic. EDIT: "op_mini all" is included via "> 1%", as the caniuse data currently has it at 1.08%. |
Turning off auto-merge to get review of my own change to the PR. |
`node-sass` is deprecated and sort-of depends on being able to download precompiled binaries that match the version of node in use. `sass` is the replacement. The update to calypso-build 9.0.0 in #20789 took care of most of the dependencies on `node-sass` in the monorepo. This cleans up the rest.
Looks great @anomiex - thanks for getting to the bottom of it. I did think a few times "how the hell did this ever work, and how does that other thing still work, and..." but you managed to get to the bottom of it with that op_mini thing. |
Great news! One last step: head over to your WordPress.com diff, D65812-code, and commit it. Thank you! |
`node-sass` is deprecated and sort-of depends on being able to download precompiled binaries that match the version of node in use. `sass` is the replacement. The update to calypso-build 9.0.0 in #20789 took care of most of the dependencies on `node-sass` in the monorepo. This cleans up the rest.
`node-sass` is deprecated and sort-of depends on being able to download precompiled binaries that match the version of node in use. `sass` is the replacement. The update to calypso-build 9.0.0 in #20789 took care of most of the dependencies on `node-sass` in the monorepo. This cleans up the rest.
r230707-wpcom |
`node-sass` is deprecated and sort-of depends on being able to download precompiled binaries that match the version of node in use. `sass` is the replacement. The update to calypso-build 9.0.0 in #20789 took care of most of the dependencies on `node-sass` in the monorepo. This cleans up the rest.
* fix dashboard styling for the Backup plugin * bring back the "Cloud" icon, the path changed after the Webpack update (#20789)
We previously updated to webpack 5, plus the latest
@automattic/calypso-build
(v9.0.0) for Jetpack. This applies those same version updates, plus the latest webpack-cli, to all the places where we use them.Changes proposed in this Pull Request:
Jetpack product discussion
p1629397551142600-slack-jetpack-crew
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Nothing should change, though we may get a few new deprecation warnings.