Remove SVGs from JS bundle, #897

tomalec · 2021-07-21T17:07:01Z

Changes proposed in this Pull Request:

Remove SVGs from JS bundle, Use them as <img>s.
Add alt texts.
Remove unnecessary width, height 100% styles.

Fixes #498

Screenshots:

See below

Detailed test instructions:

Before pulling this branch
Open the following cases in separate tabs to have something to compare trunk and fix/498-svg-out
Check it looks as before (in various window sizes)
Check the alt text is fine.

Cases:

Marketing > GLA (Dashboard) > "Edit" Free Listings > step 2
/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard&subpath=%2Ffree-listings%2Fedit&pageStep=2

Google Shopping search results example
Marketing > GLA (Dashboard) > "Add paid campaign" > complete setup
/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard&guide=campaign-creation-success

Drawing of a person who successfuly launched a campaign
Disconnect Google Account using Connection test page
https://gla1.test/wp-admin/admin.php?page=connection-test-admin-page
Marketing > GLA
/wp-admin/admin.php?page=wc-admin&subpath=%2Freconnect-accounts&path=%2Fgoogle%2Fsettings

Google Logo
Disconnect all accounts at /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsettings
Marketing > GLA
/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fstart

Google Shopping search results example
Drawing of jigsaw puzzles connecting together
Drawing of a person looking at their mobile
Drawing of a bar and line charts heading up
Complete the setup
/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fproduct-feed&guide=submission-success

WooCommerce Logo
Google Logo

Changelog entry

Tweak - Removed SVGs from JS bundle

Additional notes:

I'm not sure if exposing /js/src/... as the assets directory is the safest and fits WordPress conventions. At least it does not require moving files around and lets us keep images close to components that use them.
I think that the ideal and the cheapest for an end-user solution would be to inline SVGs into HTML. However, AFAIK we do not use any SSR, and React component instances are 100% JS (as opposed to Custom Elements, which could be served as pure HTML for the first-page load).
I made up alt text by myself, so I'd appreciate an approval for the copy (in quote blocks below respective screenshots), @j111q?
According to bundlewatch our index.js bundle got down by ~52% (719,79KB -> 347.81KB)

Use them as `<img>`s. Add alt texts. Remove unnecessary width, height 100% styles. Fixes #498

eason9487

🎉 for the bundle size! 👍

💅
Looks like we don't need this SVG mock anymore:

google-listings-and-ads/jest.config.js

Line 23 in c0ecd30

'\\.svg$': '<rootDir>/tests/mocks/assets/svgrMock.js',

(Just found that the current test did not cover the importing of SVG files, but there is no need to make up then.)

🚧
The archived zip file would not include js/src/ directory.

I'm not sure if exposing /js/src/... as the assets directory is the safest and fits WordPress conventions. At least it does not require moving files around and lets us keep images close to components that use them.

Probably we have to move all SVGs. If keep SVGs at their current positions, it would need to list all SVGs' paths in composer config.

We could consider a solution of centralizing the assets files by below steps:

Create a assets directory at project root

Add "!/assets" to the exclude list for including the assets directory into the archived zip file

google-listings-and-ads/composer.json

Lines 62 to 68 in c0ecd30

    
           "archive": { 
        
             "exclude": [ 
        
               "!/js/build", 
        
               "!/vendor/*", 
        
               "!/languages" 
        
             ] 
        
           },

Replace the assetsURL value with $this->get_plugin_url( 'assets/' )

google-listings-and-ads/src/Admin/Admin.php

Lines 111 to 113 in c0ecd30

    
           'glaData', 
        
           [ 
        
           	'assetsURL'           => plugin_dir_url( $this->get_main_file() ),

Move SVGs to assets directory and then they could be accessed via /wp-content/plugins/google-listings-and-ads/assets/[filename].svg path
Run npm run archive to see if they're included into zip file as well

eason9487 · 2021-07-22T08:02:38Z

Create a assets directory at project root

Just noticed that the path /wp-content/plugins/google-listings-and-ads/assets/google-logo.svg will be blocked by some ad-block browser extensions. 🤦‍♂️ And images wouldn't be blocked.

js/src/get-started-page/features-card/index.js

eason9487 · 2021-07-22T08:40:25Z

js/src/components/account-card/index.js

@@ -23,7 +32,14 @@ export const APPEARANCE = {

 const appearanceDict = {
 	[ APPEARANCE.GOOGLE ]: {
-		Logo: GoogleLogo,
+		Logo: (


💅
How about rename to logo since it's changed to an instantiated component?

This made me think, that serving an instance is not the best solution as if we are going to create two instances of <AccountCard appearance="google">, the Logo instance would not be duplicated and appear only in one. Do I get JSX right? I just got used to using HTMLTemplateElements and instantiating them, and JSX is not a <template> ;)

hm... 😕 it seems to eventually stamp two separate instances of <img>.

Addressed in 1ead489

Sorry, I used an imprecise term. It should be called react element if follow the definitions of React official blog. And the instance should be referred to the component instance in runtime and it keeps references to relevant DOM.

Addresses #897 (comment)

as they are now loaded as images. Addresses #897 (review)

Addresses #897 (review).

tomalec · 2021-07-22T13:06:34Z

🚧
The archived zip file would not include js/src/ directory.

Thank you for catching this! 🙏

I'd like to try to keep the files where they are, as it reduced the need for manual syncing assets used by components => reduced the costs and risk of maintaining abandoned files.

I decided to try this path.

If keep SVGs at their current positions, it would need to list all SVGs' paths in composer config.

by adding "!/js/src/**/*.svg", to composer.json.

WDYT about it, does it pass ad blockers you checked?

tomalec · 2021-07-22T13:06:53Z

I think it's ready for another review.

eason9487 · 2021-07-23T07:04:35Z

❓

I'd like to try to keep the files where they are, as it reduced the need for manual syncing assets used by components => reduced the costs and risk of maintaining abandoned files.

Abandoned files maintainability

By declaring SVGs in constants or another dedicated file, and export them directly, the maintenance cost should be very small.

// constants.js
const { assetsURL } = glaData;

export const svgGoogleLogo = assetsURL + 'gogole-logo.svg';
export const svgHero = assetsURL + 'google-free-listing.svg';

// Consumer side

import { svgGoogleLogo } from '.~/constants';

When checking whether a file is still in use, we could:

Global search the constant \bsvgGoogleLogo\b if editor supports regexp search
Comment out export to see if a warning shows up from the webpack building

Code maintainability

Regardless of whether the variables are declared in the components that use them, or in the constants.js, only the filenames are required, or at most additional subpath in the assets directory.

Compare with the current method, it can eliminate the need for comments scattered in several places and the need to write the file paths under js/src individually as below.

/**
 * Full URL to the Google G logo image.
 * Preferably we would inline it into HTML to reduce the bundle size.
 *
 * Unfortunately, React does not support `import.meta`, so we need to hardcode the module path.
 */
const googleLogoURL =
	glaData.assetsURL + 'js/src/components/account-card/gogole-g-logo.svg';

In terms of overall maintainability, such as adding new images (not only svg, maybe png or jpg, etc), moving files, and those additional comments, I think it's worth it to do a one-time move. WDYT?

eason9487 · 2021-07-23T07:18:13Z

💅
Thanks for adding this c101d69. I have one more small nitpick that the tests/mocks/assets/svgrMock.js file could be removed as well.

tomalec · 2021-07-27T09:38:35Z

Abandoned files maintainability

[…]
When checking whether a file is still in use, we could:

Sorry, I didn't state that clearly, but my goal was to remove the need of "checking whether a file is still in use" totally. Consider the scenario:

It happens that <AccountCard> is no longer needed. As a developer, all I need to do is:

Remove the js/src/components/account-card/ folder

(our linters will make sure nothing is importing anything from there)

As opposed to the solution you propose where

Remove the js/src/components/account-card/ folder.
Check for any images imported by any file in js/src/components/account-card/
1. For every image:
  1. Global search the constant \bvariableName\b if editor supports regexp search
  2. Comment out export to see if a warning shows up from the webpack building
  3. Remove the export from '.~/constants' if there is nobody else using them

So to me "By declaring SVGs in constants or another dedicated file, and export them directly, the maintenance cost" when removing the component would increase from O(1) to O(n^2)

Code maintainability

Regardless of whether the variables are declared in the components that use them, or in the constants.js, only the filenames are required, or at most additional subpath in the assets directory.

Sure, only filenames are required. But in my opinion, additional folder structure gives valuable information. By putting everything to a single flat bag of /images we would reduce discoverability and identification of images. Just like we don't put all *.js files in a single /js folder, we shouldn't put all *.(svg|png) files in a single folder. Especially, that none of those images is shared as itself. Only the components are shared.

Compare with the current method, it can eliminate the need for comments scattered in several places and the need to write the file paths under js/src individually as below.

I agree. Moreover, this comment might have some sense for a single-use, but repeating it everywhere, and complaining that React does not follow ECMA standards, and produces JS instead of HTML, does not bring any value. I'll remove them.

as repeatedly complaining that React does not follow ECMA standards, and produces JS instead of HTML, does not bring much value. To partially address #897 (comment)

Addresses #897 (comment).

tomalec · 2021-07-27T09:46:24Z

Another thing to note in favor of ~/js/src/components/**/*.(svg|png) and against /images/*.(svg|png) is that:

it keeps files closer to where they are used,
makes component more atomic, independent, and self-sufficient

eason9487

I have got @tomalec's perspective from #897 (comment). Considering that the current implementation has already addressed the issue #498, I think there's no blocker for now.

eason9487 · 2021-07-28T11:50:34Z

@tomalec

🚧

Sorry, I just noticed that the google-listings-and-ads.zip file should be about the same size as the original, but the reduced file size is almost the same as the index.js. After running npm run archive on my end, the archive file doesn't include the src/js directory. I suspected that the rule of archive.exclude doesn't support ** to find files recursively.

google-listings-and-ads/composer.json

Line 70 in 55ba670

"!/js/src/**/*.svg",

After deeper investigation about composer archive, here are my findings:

It applies rules in .gitignore to exclude files.
It excludes files in .gitattributes which are marked with export-ignore.
It applies the archive.exclude rules in composer.json to exclude files, or to include files forcibly if the rule starts with !.
- composer converts the archive.exclude glob rules to RegExp via symfony/finder library.
  https://github.com/composer/composer/blob/2.1.5/src/Composer/Package/Archiver/BaseExcludeFilter.php#L123-L147
- The required symfony/finder version of composer spans several different major versions.
  https://github.com/composer/composer/blob/2.1.5/composer.json#L37
- Newer versions of symfony/finder support ** but the latest composer.phar is still using symfony/finder v2.8.52. And I think it is the root cause that ** is not working.
  https://github.com/symfony/Finder/blob/v2.8.52/Glob.php

I haven't look into it but seems the file-loader might be another solution if want to keep images stay with needed components.

tomalec · 2021-07-28T14:00:38Z

@eason9487 I'm sorry, I missed it again :(
Thank you for finding that and an awesome investigation.

tomalec · 2021-07-28T15:43:38Z

To add to your investigation composer archive does not allow to set $strictWildcardSlash = false, to allow to mimic recursive wildcard https://github.com/symfony/Finder/blob/5.3/Glob.php#L79.

Thanks for finding file-loader -> asset-modules.
Usually, I'm against over-webpackign everything. Especially, using more webpack to solve problems created by using webpack. (If we would write build-less, or at least webpack-less, and use vanilla JS ES Modules, we could use import.meta to get the current path).

But, given where we are, this seems to be a nice solution for our problem:

lets us keep source files close to the code,
could preserve or change the path of distributed assets
could generate a content hash, which would allow merchants to enable strict caching rules.
That hurts my old-school web-dev sentiment of a browser running same files I use during development. And could make production debugging a tad complicated.
We can emit paths using the original path and filename, like: /images/{path}/{contenthash}.{filename}.{ext}. That way the files would be still matchable by a human, and the merchant would gain caching possibilities.

The only doubt I have is that we are still on Webpack 4, so we are forced to use something that we now are deprecated and has a cleaner alternative in the future. That kind of introduces additional technical debt.

Redo #897 Fixes #498 and adds `.scg`s to the `.zip` package #897 (comment)

Remove SVGs from JS bundle,

c0ecd30

Use them as `<img>`s. Add alt texts. Remove unnecessary width, height 100% styles. Fixes #498

tomalec requested a review from a team July 21, 2021 17:07

tomalec self-assigned this Jul 21, 2021

eason9487 reviewed Jul 22, 2021

View reviewed changes

tomalec added 4 commits July 22, 2021 14:23

Merge branch 'trunk' into fix/498-svg-out

80fd2d3

Change appearanceDict[*].logo case to indicate it's an instance.

1ead489

Addresses #897 (comment)

Remove SVG module mapper,

c101d69

as they are now loaded as images. Addresses #897 (review)

Add SVGs to the zip archive.

c5de2c3

Addresses #897 (review).

tomalec requested a review from eason9487 July 22, 2021 13:06

tomalec added 2 commits July 27, 2021 11:41

Remove redundant code comments,

c1832c0

as repeatedly complaining that React does not follow ECMA standards, and produces JS instead of HTML, does not bring much value. To partially address #897 (comment)

Remove not used Svgr mock.

8ac7159

Addresses #897 (comment).

eason9487 approved these changes Jul 27, 2021

View reviewed changes

tomalec merged commit 55ba670 into trunk Jul 27, 2021

tomalec deleted the fix/498-svg-out branch July 27, 2021 13:58

tomalec added a commit that referenced this pull request Jul 28, 2021

Use file-loader to expose image assets from js/src/ to js/build.

af0760a

Redo #897 Fixes #498 and adds `.scg`s to the `.zip` package #897 (comment)

This was referenced Jul 28, 2021

Add SVG to the .zip (redo #897) #913

Merged

Don't bundle SVG Images in JS build #498

Closed

	"archive": {
	"exclude": [
	"!/js/build",
	"!/vendor/*",
	"!/languages"
	]
	},

	'glaData',
	[
	'assetsURL' => plugin_dir_url( $this->get_main_file() ),

Remove SVGs from JS bundle, #897

Remove SVGs from JS bundle, #897

Uh oh!

Conversation

tomalec commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Cases:

Changelog entry

Additional notes:

Uh oh!

eason9487 left a comment

Choose a reason for hiding this comment

Uh oh!

eason9487 commented Jul 22, 2021

Uh oh!

Uh oh!

eason9487 Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

tomalec Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

tomalec Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

tomalec Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

eason9487 Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

tomalec commented Jul 22, 2021

Uh oh!

tomalec commented Jul 22, 2021

Uh oh!

eason9487 commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Abandoned files maintainability

Code maintainability

Uh oh!

eason9487 commented Jul 23, 2021

Uh oh!

tomalec commented Jul 27, 2021

Abandoned files maintainability

Code maintainability

Uh oh!

tomalec commented Jul 27, 2021

Uh oh!

eason9487 left a comment

Choose a reason for hiding this comment

Uh oh!

eason9487 commented Jul 28, 2021

Uh oh!

tomalec commented Jul 28, 2021

Uh oh!

tomalec commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tomalec commented Jul 21, 2021 •

edited

Loading

eason9487 commented Jul 23, 2021 •

edited

Loading

tomalec commented Jul 28, 2021 •

edited

Loading