Skip to content
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

Open
wants to merge 40 commits into
base: development
Choose a base branch
from

Conversation

Soare-Robert-Daniel
Copy link
Contributor

@Soare-Robert-Daniel Soare-Robert-Daniel commented Nov 15, 2023

Summary

  1. Updated NPM packages to their latest version 🚀
  2. Using wp-scripts built-in build for JS files instead of a custom one with ts-loader.
  3. Update the Rollup config files. File renamed to .mjs and sourceMap to sourcemap.
  4. The build can be run with Node 16, but Node 18 is the recommended way.
  5. Remove unused libs like ts-loader, css-loader, etc.

ℹ️ The newer version of Eslint is more strict, so we have new warnings like:

/Users/robert/Desktop/sites/dev/app/public/wp-content/themes/neve/assets/apps/customizer-controls/src/rich-text/RichTextComponent.js
  49:5  warning  React Hook useEffect has missing dependencies: 'isVisible' and 'section'. Either include them or remove the dependency array                     react-hooks/exhaustive-deps
  58:5  warning  React Hook useEffect has missing dependencies: 'controlId', 'isVisible', and 'updateValues'. Either include them or remove the dependency array  react-hooks/exhaustive-deps

/Users/robert/Desktop/sites/dev/app/public/wp-content/themes/neve/assets/apps/customizer-controls/src/spacing/SpacingComponent.js
  102:5  warning  React Hook useEffect has missing dependencies: 'control.id', 'defaultValue', 'dependsOn', and 'updateControlValue'. Either include them or remove the dependency array  react-hooks/exhaustive-deps

/Users/robert/Desktop/sites/dev/app/public/wp-content/themes/neve/assets/apps/customizer-controls/src/toggle/ToggleComponent.js
  17:5  warning  React Hook useEffect has missing dependencies: 'control.id', 'toggleValue', and 'value'. Either include them or remove the dependency array  react-hooks/exhaustive-deps

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:

Old Babel Version
  proposal-numeric-separator { "ios":"12.2" }
  proposal-logical-assignment-operators { "firefox":"78", "ios":"12.2", "opera":"88", "samsung":"17" }
  proposal-nullish-coalescing-operator { "ios":"12.2" }
  proposal-optional-chaining { "ios":"12.2" }
  syntax-json-strings { "android":"103", "chrome":"102", "edge":"103", "firefox":"78", "ios":"12.2", "opera":"88", "safari":"15.5", "samsung":"17" }
  syntax-optional-catch-binding { "android":"103", "chrome":"102", "edge":"103", "firefox":"78", "ios":"12.2", "opera":"88", "safari":"15.5", "samsung":"17" }
  syntax-async-generators { "android":"103", "chrome":"102", "edge":"103", "firefox":"78", "ios":"12.2", "opera":"88", "safari":"15.5", "samsung":"17" }
  syntax-object-rest-spread { "android":"103", "chrome":"102", "edge":"103", "firefox":"78", "ios":"12.2", "opera":"88", "safari":"15.5", "samsung":"17" }
  transform-template-literals { "ios":"12.2" }
  proposal-export-namespace-from { "firefox":"78", "ios":"12.2", "safari":"15.5" }
  syntax-dynamic-import { "android":"103", "chrome":"102", "edge":"103", "firefox":"78", "ios":"12.2", "opera":"88", "safari":"15.5", "samsung":"17" }
  
 New Babel Version:
  transform-unicode-sets-regex { chrome < 112, firefox < 116, ios, opera_mobile < 75, safari < tp, samsung }
  proposal-class-static-block { ios < 16.4, safari < 16.4 }
  syntax-private-property-in-object
  syntax-class-properties
  syntax-numeric-separator
  syntax-nullish-coalescing-operator
  syntax-optional-chaining
  syntax-json-strings
  syntax-optional-catch-binding
  syntax-async-generators
  syntax-object-rest-spread
  syntax-export-namespace-from
  syntax-dynamic-import
  syntax-top-level-await
  syntax-import-meta 

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 for document to reduce the size. It worked 🔧 . But for shop.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 with 234 B -- for this, I think we need to increase the limit 😢

⚠️ Yarn changed with NPM

The new version of Eslint is not working with Yarn. It gives an error like:

yarn run lint
yarn run v1.22.21
$ npm-run-all lint:*
$ wp-scripts lint-js ./assets/js/src/
There was a problem loading formatter: /Users/robert/Desktop/sites/dev/app/public/wp-content/themes/neve/node_modules/eslint/lib/cli-engine/formatters/stylish
Error: require() of ES Module /Users/robert/Desktop/sites/dev/app/public/wp-content/themes/neve/node_modules/strip-ansi/index.js from /Users/robert/Desktop/sites/dev/app/public/wp-content/themes/neve/node_modules/eslint/lib/cli-engine/formatters/stylish.js not supported.
Instead change the require of index.js in /Users/robert/Desktop/sites/dev/app/public/wp-content/themes/neve/node_modules/eslint/lib/cli-engine/formatters/stylish.js to a dynamic import() which is available in all CommonJS modules.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "lint:global" exited with 2.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

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.

  1. Make two instances, one with the latest version of WP and another with WP5.5 (our minimum). Also, install Neve Pro.
  2. Load the version from the PR and check for errors.

⚠️ When encountering an error, please post a screenshot an share the instance. We need to double-check to see if it is a new error introduced in this or if it also happened in the current version of Neve, and we just found out about it.

Check before Pull Request is ready:

Closes #4143

@Soare-Robert-Daniel Soare-Robert-Daniel added the pr-checklist-skip Allow this Pull Request to skip checklist. label Nov 15, 2023
@Soare-Robert-Daniel Soare-Robert-Daniel self-assigned this Nov 15, 2023
@Soare-Robert-Daniel Soare-Robert-Daniel marked this pull request as ready for review November 15, 2023 15:12
@pirate-bot pirate-bot added the pr-checklist-complete The Pull Request checklist is complete. (automatic label) label Nov 15, 2023
@pirate-bot
Copy link
Collaborator

pirate-bot commented Nov 15, 2023

Plugin build for 24410c1 is ready 🛎️!

@Soare-Robert-Daniel Soare-Robert-Daniel linked an issue Nov 15, 2023 that may be closed by this pull request
@Soare-Robert-Daniel
Copy link
Contributor Author

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)

@preda-bogdan
Copy link
Collaborator

@Soare-Robert-Daniel what is the status on this PR?

@Soare-Robert-Daniel
Copy link
Contributor Author

@preda-bogdan, the PR is done. I waiting for the update on E2E regarding visual regression. Everything should be green before we start testing.

@GrigoreMihai
Copy link
Contributor

GrigoreMihai commented Feb 7, 2024

@preda-bogdan I agree with you on the fact that there are a lot of changes at once and will be hard to track down a bug if it reaches production.

What I'm thinking is if we focus on adding more automated tests before this update so we will reach a point where we are more confident that changes don't introduce bugs? But this might take a while.

@Soare-Robert-Daniel What I'm thinking if it will be possible to split this into multiple parts ? And let's say we leave the babel update at the very end since it is adding a bit of size.

Also for babel would it be possible to check it's config options https://babeljs.io/docs/options to see if there are features we can disable during bundling (like the ones that we don't need from the new added ones) ? My idea would still be that an package update should not increase the bundle size if we keep the same config, of course if you think it's possible.

@Soare-Robert-Daniel
Copy link
Contributor Author

The waryness about the build size being a few bytes more is not translated to possible error.

The new build has just another arrangement.

Example:

Screenshot 2024-02-07 at 17 24 26

Here, you can see the new version moves the var outside the for loop and no longer creates the o var, which takes the value of t.lenght.

Why does it do that? It does because that var additional var is not needed. It is an unused var; you can check the code here: https://github.com/ganlanyuan/tiny-slider/blob/4d709735c417c2483e77a22d017fc1b18c04f0d4/src/helpers/whichProperty.js#L16-L21

As you can see, the new version structure is more similar to the original code.

But this similarity is not cheap since we now have two declarations of var keyword with it its needed space between keyword and var

Another example is this one:

Screenshot 2024-02-07 at 17 15 15

As you can see, it is just a different arrangement.

Same thing here:

Screenshot 2024-02-07 at 17 36 57

The new build uses a var to store the string. Like in the source code https://github.com/ganlanyuan/tiny-slider/blob/4d709735c417c2483e77a22d017fc1b18c04f0d4/src/tiny-slider.module.js#L375C27-L375C43

The old build just inlined the string where it was used:

Screenshot 2024-02-07 at 17 39 27

@Soare-Robert-Daniel
Copy link
Contributor Author

If you want to see more details check the zip file with the build file from the master vs the one in this PR. All of them were formatted to have a nice visualization with diff

neve-build-compare.zip

@Soare-Robert-Daniel
Copy link
Contributor Author

Also if you check other build files, you can find intresting things like this:

Screenshot 2024-02-07 at 18 03 20

!l.toString.toString().includes("[native code]") is a check to make sure that the base function was not tempered.

@Soare-Robert-Daniel
Copy link
Contributor Author

Soare-Robert-Daniel commented Feb 27, 2024

Now, the shop.js is below the limit. No need for a new limit 🔥

Since all the shop.js dependencies do not have classes or abuse of this, we can be more aggressive with the use of the function arrow:

image

image

image

The save is not great, but not terrible:

  assets/js/build/modern/frontend.js
  Size limit:   7 kB
  Size:         6.93 kB with all dependencies and minified
  Loading time: 136 ms  on slow 3G
  
  assets/js/build/modern/shop.js
  Size limit:   33 kB
  Size:         32.67 kB with all dependencies and minified
  Loading time: 639 ms   on slow 3G
  
  style-main-new.min.css
  Size limit:   38.1 kB
  Size:         36.99 kB with all dependencies and minified
  Loading time: 723 ms   on slow 3G

Copy link
Contributor

@GrigoreMihai GrigoreMihai left a 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.

Copy link
Collaborator

@preda-bogdan preda-bogdan left a 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.

@irinelenache
Copy link
Contributor

irinelenache commented Apr 17, 2024

@Soare-Robert-Daniel Did some testing and found a single issue for now:

  • After activating all items in the post meta for Blog/archive, the meta wasn't visible anymore neither on the blog page or in a single post . That happened using only Neve and importing the Pet shop starter site. When I tried to reorder the post meta, they started to work again.
  • Something similar happened when I added another icon in the Social icons header component. They were visible again after reordering https://vertis.d.pr/v/KkoMMt
  • ^ The same happened for the Payment icons in the footer

Testing is not complete so I'll add other comments if I find anything else

@irinelenache
Copy link
Contributor

Can be checked on this instance:

https://packageupdate.s1-tastewp.com/wp-admin/
user: test
password: test

@Soare-Robert-Daniel
Copy link
Contributor Author

@irinelenache, thanks a lot for the testing.

@Soare-Robert-Daniel Soare-Robert-Daniel marked this pull request as draft April 19, 2024 09:38
@Soare-Robert-Daniel Soare-Robert-Daniel marked this pull request as ready for review April 23, 2024 12:58
@Soare-Robert-Daniel
Copy link
Contributor Author

  • After activating all items in the post meta for Blog/archive, the meta wasn't visible anymore neither on the blog page or in a single post . That happened using only Neve and importing the Pet shop starter site. When I tried to reorder the post meta, they started to work again.
  • Something similar happened when I added another icon in the Social icons header component. They were visible again after reordering https://vertis.d.pr/v/KkoMMt
  • ^ The same happened for the Payment icons in the footer

Those are now fixed 🔥

The version in npm package for react-sortablejs was set to ^6.0.0, and when I did the update, the lock file was saved, a newer version ( the yarn lock had the 6.0.0).

What was the problem? The component used for sorting the icons augments the given list (in our case, the var list with additional keys (selected, chosen)), and we also use list to send the updates to the db.

Until version 6.0.4, the component made a shallow copy of each item along with the augmentation. The functions that were working directly with sorting functions were aware of this behavior and cleaned up the keys because otherwise, the server sanitization would reject it.(that's why sorting was working as expected)

On version 6.0.4, the devs changed the behavior so that the sorting component will preserve the original list. Thus, the augmented keys were added to the initial list, poisoning all the functions (like the Add Item button).

To solve this, I pinned down to version to 6.0.3 to always install this version regarding how you run it with npm or yarn.

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.

@Soare-Robert-Daniel
Copy link
Contributor Author

This happens also in the current version. Made an issue about it https://github.com/Codeinwp/neve-pro-addon/issues/2804

@irinelenache
Copy link
Contributor

@Soare-Robert-Daniel The issues are fixed now, thank you ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-checklist-complete The Pull Request checklist is complete. (automatic label) pr-checklist-skip Allow this Pull Request to skip checklist.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dev] Update NPM packages and JS build
5 participants