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

Scripts: add custom TerserPlugin configuration #22990

Merged
merged 5 commits into from
Jul 6, 2020

Conversation

swissspidy
Copy link
Member

Description

Updates webpack configuration to preserve translator comments in minified output.

Fixes #19006.

How has this been tested?

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@swissspidy swissspidy added [Type] Enhancement A suggestion for improvement. [Tool] WP Scripts /packages/scripts labels Jun 8, 2020
@github-actions
Copy link

github-actions bot commented Jun 8, 2020

Size Change: +970 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-directory/index.js 7.63 kB +153 B (2%)
build/block-editor/index.js 110 kB +723 B (0%)
build/block-library/index.js 130 kB -65 B (0%)
build/block-serialization-spec-parser/index.js 3.1 kB -5 B (0%)
build/blocks/index.js 48.2 kB +73 B (0%)
build/components/index.js 199 kB +79 B (0%)
build/compose/index.js 9.56 kB -89 B (0%)
build/date/index.js 5.38 kB -89 B (1%)
build/edit-navigation/index.js 9.92 kB -57 B (0%)
build/edit-post/index.js 304 kB +137 B (0%)
build/edit-site/index.js 16.7 kB +23 B (0%)
build/edit-widgets/index.js 9.35 kB +26 B (0%)
build/editor/index.js 45 kB +193 B (0%)
build/media-utils/index.js 5.32 kB +22 B (0%)
build/primitives/index.js 1.41 kB -92 B (6%)
build/rich-text/index.js 13.9 kB -99 B (0%)
build/server-side-render/index.js 2.71 kB +37 B (1%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.68 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.57 kB 0 B
build/block-library/editor.css 7.57 kB 0 B
build/block-library/style-rtl.css 7.78 kB 0 B
build/block-library/style.css 7.79 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.8 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.19 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/style-rtl.css 5.57 kB 0 B
build/edit-post/style.css 5.57 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.77 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.73 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.41 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@swissspidy swissspidy requested review from gziolo, aduth and ocean90 June 9, 2020 10:03
@swissspidy
Copy link
Member Author

Just adding some reviewers to know whether I'm on the right track here. Odd that the bundle size didn't change, but I thought I saw the comments locally 🤔

@gziolo gziolo requested review from jsnajdr and sgomes June 10, 2020 05:56
@gziolo
Copy link
Member

gziolo commented Jun 10, 2020

Odd that the bundle size didn't change, but I thought I saw the comments locally 🤔

We use two different configs for the Gutenberg plugin and npm package. So we should either duplicate logic or create a webpack plugin that optimizes i18n handling.

@swissspidy swissspidy force-pushed the add/wp-scripts-terser-plugin branch from 36a5ca3 to 38e8d6b Compare June 10, 2020 07:12
@swissspidy swissspidy marked this pull request as ready for review June 10, 2020 07:58
@@ -73,6 +74,19 @@ const config = {
default: false,
},
},
minimizer: [
new TerserPlugin( {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought webpack uses terser by default, do we need the extra plugin to configure it properly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does with the default options. But to customize the options you have to pass your own TerserPlugin instance.

Copy link
Member

@ocean90 ocean90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work for me. Steps to test:

  • Run npm run package-plugin
  • Extract the ZIP file and cd into the new gutenberg directory
  • Run php -d memory_limit=512M "$(which wp)" i18n make-pot . --ignore-domain (requires WP-CLI to be installed)
  • Open gutenberg.pot in an editor
  • Search for (%s: color %s)

Result:

#. translators: first %s: The type of color or gradient (e.g. background, overlay...), second %s: the color name or value (e.g. red or #ff0000)
#: build/block-editor/index.js:6
msgid "(%s: color %s)"
msgstr ""

(Unrelated to this PR but this should use numbered placeholders.)

The source file looks like this:
build-block-editor-index js

The only thing I'm not sure about is whether this should be part of @wordpress/scripts or not since it appears to only affect the plugin itself due to not including the source files in the plugin.

@youknowriad
Copy link
Contributor

We probably need a similar patch on Core right?

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are better testers than me, I confirmed the comments are kept on the minified files :)

@ocean90
Copy link
Member

ocean90 commented Jun 10, 2020

We probably need a similar patch on Core right?

Core already bundles an unminified version of the files which include the comments, see wp-includes/js/dist/blocks.js for example.

package-lock.json Outdated Show resolved Hide resolved
@swissspidy
Copy link
Member Author

The only thing I'm not sure about is whether this should be part of @wordpress/scripts or not since it appears to only affect the plugin itself due to not including the source files in the plugin.

Any plugin developer using @wordpress/scripts and hosting their plugin on WordPress.org will benefit from this too.

Core already bundles an unminified version of the files which include the comments

Is this done via some custom implementation? If so, can you remove it now that the built packages will include the comments out oft the box?

@swissspidy swissspidy requested a review from gziolo June 11, 2020 09:08
@gziolo
Copy link
Member

gziolo commented Jul 4, 2020

If needs a rebase now and you can merge it afterwards.

sourceMap: ! isProduction,
terserOptions: {
output: {
comments: /translators:/i,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just found this. Why would we want to keep the translators comments in our production builds? I thought they're only used on our source code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are needed because the production builds are the ones that are scanned for any translation function calls and made available on translate.wordpress.org.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really all explained in the ticket this PR addresses: #19006

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool! Probably worth a comment here though. Thanks for the info 👍!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

i18n: Configure minification to retain translator comments
5 participants