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

Support parsing minified JS files created by WebPack #85

Merged
merged 9 commits into from Oct 29, 2018

Conversation

4 participants
@herregroen
Copy link
Contributor

herregroen commented Oct 24, 2018

In order to resolve https://meta.trac.wordpress.org/ticket/3875 wp i18n make-pot should be able to traverse minified files created by WebPack in order to find where each translation is used, this so that when generating language packs it can generate a file for each JS file containing only the necessary translations.

Because WebPack wraps calls to wp.i18n.__( "string" ) in the following form Object(u.__)( "string" ) this adds support for CallExpressions where the callee is a CallExpression itself whose callee is Object and argument the necessary translation function.

This part will likely later need to be expanded to support more forms in which translations may be called. Supporting MemberExpression as the callee for example would be an obvious first improvement. Support for MemberExpression as the callee has been added in the PR.

@herregroen herregroen force-pushed the herregroen:master branch from e85b254 to fcfba1e Oct 24, 2018

@schlessera
Copy link
Member

schlessera left a comment

I'd like to see 2 changes before merging this:

  1. The code that parses the minified JS needs tests in the features/makepot.feature file. Otherwise, there's no way to guard from regressions.

  2. Instead of having this big if/elseif/elseif, I'd like to have individual methods that describes what is currently being tested for. Inside of these methods, the code can be much cleaner with intermediate variables and early returns.

@herregroen

This comment has been minimized.

Copy link
Contributor

herregroen commented Oct 25, 2018

@schlessera Thanks for the review!

I've added tests to makepot.feature. These are currently simply added to existing JS parsing test as especially following MemberExpression callees ( for example: wp.i18n.__ ) aren't exclusive to minified files.

The WebPack specific format ( Object(u.__)( 'translation' ) ) is a lot more niche so let me know if you feel there should be a separate scenario for that.

The added tests fail when I revert the changes to src/JsFunctionsScanner.php so should guard from regressions.

Resolving the callee has also been split to it's own function with more extensive documentation and examples.

@herregroen herregroen force-pushed the herregroen:master branch from 51ed9fb to ff794d9 Oct 25, 2018

@herregroen herregroen force-pushed the herregroen:master branch from ff794d9 to 918206a Oct 25, 2018

'CallExpression' === $node->getCallee()->getType() &&
'Identifier' === $node->getCallee()->getCallee()->getType() &&
'Object' === $node->getCallee()->getCallee()->getName() &&
! empty( $node->getCallee()->getArguments() ) &&

This comment has been minimized.

@schlessera

schlessera Oct 25, 2018

Member

This fails on PHP 5.4: https://travis-ci.org/wp-cli/i18n-command/jobs/446078984#L633

empty() needs to work on a reference for PHP 5.4 and lower, so you cannot directly use a method call here.

This comment has been minimized.

@schlessera

schlessera Oct 25, 2018

Member

Alternatively, you can change to test to not use empty(), but compare to an empty array (which I think is the expected return value here).

@swissspidy

This comment has been minimized.

Copy link
Collaborator

swissspidy commented Oct 25, 2018

Instead of parsing *.min.js by default, I think we should leave the exclude rule as-is, but make it possible to include them.

For this we'd need to swap the include and exclude check as discussed in #84. I'm gonna try to merge the unit tests for that (#70) and then attempt modifying IterableCodeExtractor::getFilesFromDirectory() accordingly.

@@ -41,7 +41,7 @@ class MakePotCommand extends WP_CLI_Command {
/**
* @var array
*/
protected $exclude = [ 'node_modules', '.git', '.svn', '.CVS', '.hg', 'vendor', 'Gruntfile.js', 'webpack.config.js', '*.min.js' ];
protected $exclude = [ 'node_modules', '.git', '.svn', '.CVS', '.hg', 'vendor', 'Gruntfile.js', 'webpack.config.js' ];

This comment has been minimized.

@schlessera

schlessera Oct 25, 2018

Member

As #87 was now merged, this change is not needed anymore.

By default, minified files should still be excluded, and if you want to explicitly include them, you can do so by adding --include=*.min.js to the command.

This comment has been minimized.

@herregroen

herregroen Oct 26, 2018

Contributor

Awesome! Will remove this in that case.

Show resolved Hide resolved features/makepot.feature
Show resolved Hide resolved src/JsFunctionsScanner.php Outdated

schlessera and others added some commits Oct 26, 2018

Webpack with lowercase p.
Co-Authored-By: herregroen <herregroen@gmail.com>
Show resolved Hide resolved src/JsFunctionsScanner.php Outdated

@swissspidy swissspidy added this to the 2.0.2 milestone Oct 29, 2018

@ocean90

This comment has been minimized.

Copy link
Contributor

ocean90 commented Oct 29, 2018

Using this PR and I can't get the JS strings of the current 5.0 branch (svn co http://core.svn.wordpress.org/branches/5.0/ ~/wordpress-50) into the POT file. I'm using this command:

php -d memory_limit=512M "$(which wp)" i18n make-pot ~/wordpress-50 ~/wordpress-50.pot --exclude="wp-admin/*,wp-content/themes/*,wp-includes/class-pop3.php,wp-content/plugins/akismet/" --package-name='WordPress' --headers='{"Report-Msgid-Bugs-To":"https://core.trac.wordpress.org/","PO-Revision-Date":"YEAR-MO-DA HO:MI+ZONE"}' --file-comment='Copyright (C) 2018 by the contributors\nThis file is distributed under the same license as the WordPress package.' --ignore-domain

Am I missing something?

@schlessera

This comment has been minimized.

Copy link
Member

schlessera commented Oct 29, 2018

@ocean90 If the files are minified, you need to include them explicitly with --include=*.min.js.

swissspidy and others added some commits Oct 29, 2018

Use modern array syntax.
Co-Authored-By: herregroen <herregroen@gmail.com>
@ocean90

This comment has been minimized.

Copy link
Contributor

ocean90 commented Oct 29, 2018

This is now working for me, after updating to the latest revisions and using the correct packages directory....

@schlessera schlessera merged commit 5b4ee94 into wp-cli:master Oct 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment