-
Notifications
You must be signed in to change notification settings - Fork 84
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
[DEV] Update NPM packages and JS build #4144
Conversation
The Gutenberg toolchain just dropped support for Node < 18 in the newest version. https://github.com/WordPress/gutenberg/blob/cdd6cecbc5bbf5ce5f55da0301246acfcf56c426/packages/scripts/CHANGELOG.md#2700-2024-01-10 The packages in this PR are all compatible with Node 18. Until this PR gets merged into the main, we will keep compatibility with Node 16 because of the Translations Diff job, which compares with the main (in this PR case, the old packages) |
@Soare-Robert-Daniel what is the status on this PR? |
@preda-bogdan, the PR is done. I waiting for the update on E2E regarding visual regression. Everything should be green before we start testing. |
…llup" This reverts commit 9d5bf3d.
Now, the Since all the The save is not great, but not terrible:
|
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.
With all the changes/explanations that were added later by Robert and since we are now also below the limit, I think this is as save as we could be with this kind of updates so I'm approving the pr, with the mention that I consider we need to carefully test the theme once everything is merged into development before a release.
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 will approve the PR so that we proceed with these changes. @Soare-Robert-Daniel make sure we have thorough manual testing and is submitted to QA before merging to development and before releasing these changes.
@Soare-Robert-Daniel Did some testing and found a single issue for now:
Testing is not complete so I'll add other comments if I find anything else |
Can be checked on this instance:
|
@irinelenache, thanks a lot for the testing. |
Those are now fixed 🔥 The version in npm package for What was the problem? The component used for sorting the icons augments the given list (in our case, the var Until version On version To solve this, I pinned down to version to Ideally, we should use a separate list for sorting and another one for storing the data that is going to the database to make sure the library is not making unwanted changes. |
This happens also in the current version. Made an issue about it https://github.com/Codeinwp/neve-pro-addon/issues/2804 |
@Soare-Robert-Daniel The issues are fixed now, thank you ✅ |
Summary
wp-scripts
built-in build for JS files instead of a custom one withts-loader
..mjs
andsourceMap
tosourcemap
.ts-loader
,css-loader
, etc.ℹ️ The newer version of Eslint is more strict, so we have new warnings like:
Build Optimizations
When new versions change, this change has slightly increased the build size, even though some things are much smaller.
For CSS, the last version of the SASS compiler can have some smart replacements like
:nth-child(even)
to:nth-child(2n)
, which saves some space. It is stuck at some basic things like removing the space in this structure:var(--a, var(--b))
, which adds up and thus makes it cross the limit.Fortunately, with a new version comes new opportunities. With CSS minifier, we can use level 2 optimization to make some smart arrangements in the selector to reuse more code. This option enabled the size to be reduced to
1kb
👌It is a big win for CSS, but not for JS...
For the JS part, it was a bit sad of a change; the new Babel version has new tricks to deal with JS, and those new features increased the size of the scripts:
As you can see, a lot of functionality in the old Babel version was at the proposal stage. And because of this, the new code slightly differs from the old one.
In the Rollup configuration file, you will see that I disabled:
'@babel/plugin-transform-parameters'
which does this -- the change was not present in the Old version.For
frontend.js
, the limit was above with under 100B, so I put a little trick like making an alias fordocument
to reduce the size. It worked 🔧 . But forshop.js
, the trick was not possible since most of the code is from a third-party lib, and the new Babel version introduced some additional vars and increased the size with234 B
-- for this, I think we need to increase the limit 😢The new version of Eslint is not working with Yarn. It gives an error like:
So I had to change to NPM, which gives proper package support.
Will affect visual aspect of the product
NO
Screenshots
Test instructions
Since it is a big update on packages, there is a possibility for broken packages. Also, we need to keep in mind the compatibility.
Check before Pull Request is ready:
Closes #4143