JavaScript String Extraction #26

swissspidy · 2018-04-27T13:11:17Z

More and more plugins rely heavily on JavaScript and the JS I18N library established by Gutenberg.

Since neither makepot.php nor this WP-CLI command support JavaScript, they had to build their own Babel plugin to extract JavaScript files.

However, one cannot expect users to a) use Babel b) use two tools to generate a simple POT file.

So, why not support this in this little WP-CLI command here? :-)

Even though the Gettext library we use for the PHP part has a JS functions scanner, it is very limited: no support for comments, no clear indication which ECMAScript standard is supported, no usage of an AST, which is really important with such a quickly evolving language.

I decided to use Peast to traverse a script's AST, which was incredibly easy. We might even consider suggesting this for the Gettext library.

Still needs some tests, but so far it works fine.

Fixes #30.

aduth · 2018-04-30T18:11:51Z

However, one cannot expect users to a) use Babel

While it's not implied, just to note that a common point of confusion is that its implementation as a Babel plugin has something to do with an opt-in decision to use newer language syntax, when in fact it was largely a combination of (a) taking advantage of the Babylon parser which, being at the core of Babel's language transpilation, is assumed to be well-maintained and (b) to combine into the same transpilation pipeline if one decides to use newer language features (as Gutenberg does, avoiding two separate parse steps).

aduth · 2018-04-30T18:13:31Z

b) use two tools to generate a simple POT file.

While it may be implemented as such in its current form, there's nothing which requires this to be a two-step process.

See #27.

swissspidy · 2018-05-09T09:25:11Z

@aduth Totally get the point. There's nothing wrong with that and I don't think we can get around that anyway.

Although JSX support is planned for Peast (the parser used in this PR), it makes more sense to use something like the Babel plugin for core and Gutenberg. That's why I've been working on things like #31 to make it easier to use both hand in hand.

As mentioned in yesterday's meeting, we'll likely test such a setup with Gutenberg and some other plugin(s) on WordPress.org soon. That way one at least won't have to generate these PHP files anymore during a build.

Useful when this is done in another build step, e.g. through Babel.

swissspidy · 2018-07-03T08:31:34Z

Did some updates today:

Fix merge conflicts
Add support for JSX
Add option to skip JS extraction altogether (see Add option to skip PHP or JS extraction altogether #30).
Useful when this is already done in another build step, e.g. through Babel.

schlessera · 2018-07-03T15:09:56Z

src/MakePotCommand.php

@@ -140,7 +153,7 @@ protected function retrieve_main_file_data() {

 		$files = new IteratorIterator( new DirectoryIterator( $this->source ) );

-		/** @var DirectoryIterator $file */
+		/* @var DirectoryIterator $file */


This should be a real docblock /** */, but inside of the foreach block as the first line, as $file only exists within the foreach block.
This will then allow an IDE to know how to handle $file in there.

@schlessera What IDE do you use? PhpStorm for example handles this just fine outside the foreach.

In WordPress core both /** and /* is used for these annotations. Not sure if WPCS would think the former is a badly formatted doc block.

I use PHPStorm as well, and for a few versions now, it interprets this correctly. But technically, the variable does not exist within the scope where you qualify it, and some other editors and static analysis tools will flag this.

I usually do one of these two instead:

foreach ( $files as $file ) { /** @var DirectoryIterator $file */ if ( $file->isFile() && $file->isReadable() && 'php' === $file->getExtension()) { $plugin_files[] = $file->getRealPath(); } }

foreach ( $files as /** @var DirectoryIterator $file */ $file ) { if ( $file->isFile() && $file->isReadable() && 'php' === $file->getExtension()) { $plugin_files[] = $file->getRealPath(); } }

Regarding the docblock:
Although PHPStorm is generally very lax in this regard, using @var inside of a regular multiline comment does not have any semantic meaning whatsoever. Tags only have a proper meaning within docblocks. Again, some tools are more strict about this and will ignore the @var if it is not within a proper docblock.

Hmm, nevermind my comments about moving the docblock inside of the foreach. This is apparently handled as an exception in the proposed PSR: https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md

However, the above standard proposal also states that tags need to be within a docblock, not a regular comment.

Good to know, thanks.

I moved it into the foreach now and use a proper docblock and everything still works :-)

schlessera · 2018-07-03T15:13:48Z

src/MakePotCommand.php

-		$this->slug   = Utils\get_flag_value( $assoc_args, 'slug', Utils\basename( $this->source ) );
+		$this->source  = realpath( $args[0] );
+		$this->slug    = Utils\get_flag_value( $assoc_args, 'slug', Utils\basename( $this->source ) );
+		$this->scan_js = ! Utils\get_flag_value( $assoc_args, 'skip-js', ! $this->scan_js );


I'd prefer the property to be called $skip_js, to avoid the weird double negation here. As it is only used once, just directly mapping the parameter seems clearer to me.

schlessera · 2018-07-03T15:17:02Z

src/JsFunctionsScanner.php

+use Peast\Syntax\Node\Node;
+use Peast\Syntax\Node\VariableDeclaration;
+use Peast\Traverser;
+use Peast\Syntax\Node\CallExpression;


All of these Node\Something imports are a case where I would prefer the use of relative namespaces.
So, have only 1 import Peast\Syntax\Node here, and then use relative namespaces like Node\Identifier or Node\Literal in the code.
This makes the import list less daunting, and it gives more context to the consuming code, where it is clear that these are all different types of nodes.

Hmm, I now see that the library has a Node\Node, which is unfortunate and might make my above suggestion weird.

I usually build my code so that the interface is outside of the namespace with the implementations. This way, you can use Node as well as Node\Literal, ...

I'll let you decide what you find cleaner, but having a Node\Node inside the code is certainly not really elegant.

schlessera · 2018-07-03T15:21:52Z

src/JsFunctionsScanner.php

+use Peast\Syntax\Node\Identifier;
+use Peast\Syntax\Node\Literal;
+
+class JsFunctionsScanner extends GettextJsFunctionsScanner {


Class should be final and protected methods/properties should be private (unless they were already protected in the superclass).

schlessera · 2018-07-03T15:22:12Z

src/PhpCodeExtractor.php

@@ -11,7 +11,7 @@
 use SplFileInfo;
 use WP_CLI;

-class WordPressCodeExtractor extends PhpCode {
+class PhpCodeExtractor extends PhpCode {


Class should be final and protected methods/properties should be private (unless they were already protected in the superclass).

schlessera · 2018-07-03T15:27:48Z

src/JsFunctionsScanner.php

+			$functions = $options['functions'];
+			$file      = $options['file'];
+
+			/* @var Node $node */


This should be a real docblock inside of the foreach block.

schlessera · 2018-07-03T15:28:02Z

src/JsCodeExtractor.php

+			RecursiveIteratorIterator::CHILD_FIRST
+		);
+
+		/* @var \DirectoryIterator $file */


This should be a real docblock inside of the foreach block.

schlessera · 2018-07-03T15:29:05Z

src/PhpCodeExtractor.php

+			RecursiveIteratorIterator::CHILD_FIRST
+		);
+
+		/* @var \DirectoryIterator $file */


This should be a real docblock inside of the foreach block.

schlessera · 2018-07-03T15:29:47Z

src/PhpCodeExtractor.php

+			RecursiveIteratorIterator::CHILD_FIRST
+		);
+
+		/* @var \DirectoryIterator $file */


This should be imported instead of using a FQCN.

schlessera · 2018-07-03T15:31:36Z

src/JsCodeExtractor.php

+			RecursiveIteratorIterator::CHILD_FIRST
+		);
+
+		/* @var \DirectoryIterator $file */


This should be imported instead of using a FQCN.

One could also argue that this would result in an unused import statement as it's only used for the code comment.

Is this defined in the coding standards somewhere?

The coding standards don't deal with namespaces, so there's no imports either.

It is not an unused import, as you're referencing it through the docblock. Note that PHP annotations like @var are considered parsable and executable code. So the annotation is something like new var( DirectorIterator::class, 'file' ).

schlessera · 2018-07-04T08:16:42Z

🎉

We will probably still need to iterate on this in the future, but having it merged now means we can better test it in actual use cases.

JavaScript String Extraction

swissspidy added 2 commits April 27, 2018 15:03

First pass at JS string extraction

e40a373

Add some failing tests for translator comments

dd032a0

swissspidy mentioned this pull request Apr 27, 2018

Add option to exclude certain folders and files #27

Closed

swissspidy added 3 commits April 30, 2018 18:09

Remove dead code

299e12e

Add some helpful debug lines

5ef5ab1

Improve comments extraction

33970bc

swissspidy requested a review from a team April 30, 2018 17:45

swissspidy mentioned this pull request May 2, 2018

Command to add text domains to i18n functions #25

Closed

swissspidy added 2 commits May 3, 2018 00:02

Simplify comments extraction

08c3a34

Skip some common JS files

9ac13fa

See #27.

This was referenced May 3, 2018

Add option to merge newly extracted strings with existing ones #29

Closed

Add option to skip PHP or JS extraction altogether #30

Closed

swissspidy added 7 commits June 19, 2018 11:08

Update mck89/peast for JSX support

5b0408e

Use some JSX in tests

dd5fdcb

Merge branch 'master' into js-code-extraction

c85c69c

Fix typo in scenario name

d5ce1ec

Enable JSX option in Peast

91004e3

Update readme

91f8fdc

Add --skip-js option

36a3876

Useful when this is done in another build step, e.g. through Babel.

schlessera requested changes Jul 3, 2018

View reviewed changes

swissspidy and others added 6 commits July 4, 2018 08:40

Rename scan_js to skip_js for increased readability

c34b0b9

Fix code annotations

32b2135

Remove obsolete todo comment

e04360a

Final & private

93a7fdd

Merge branch 'master' into js-code-extraction

f4f58ae

Add missing new-line

57a4d58

schlessera added the command:i18n-make-pot label Jul 4, 2018

schlessera added this to the 0.1.0 milestone Jul 4, 2018

swissspidy requested a review from schlessera July 4, 2018 08:11

schlessera approved these changes Jul 4, 2018

View reviewed changes

schlessera merged commit 7e11d8c into master Jul 4, 2018

schlessera deleted the js-code-extraction branch July 4, 2018 08:15

swissspidy mentioned this pull request Jul 6, 2018

Correctly pass exclude option to JsCodeExtractor #37

Merged

schlessera added a commit that referenced this pull request Jan 6, 2022

Merge pull request #26 from wp-cli/js-code-extraction

01e8a55

JavaScript String Extraction

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JavaScript String Extraction #26

JavaScript String Extraction #26

swissspidy commented Apr 27, 2018 •

edited

Loading

aduth commented Apr 30, 2018

aduth commented Apr 30, 2018

swissspidy commented May 9, 2018

swissspidy commented Jul 3, 2018

schlessera Jul 3, 2018

swissspidy Jul 4, 2018

schlessera Jul 4, 2018

schlessera Jul 4, 2018

schlessera Jul 4, 2018

swissspidy Jul 4, 2018

schlessera Jul 3, 2018

schlessera Jul 3, 2018

schlessera Jul 3, 2018

schlessera Jul 3, 2018

schlessera Jul 3, 2018

schlessera Jul 3, 2018

schlessera Jul 3, 2018

schlessera Jul 3, 2018

schlessera Jul 3, 2018

schlessera Jul 3, 2018

swissspidy Jul 4, 2018

schlessera Jul 4, 2018

schlessera commented Jul 4, 2018

JavaScript String Extraction #26

JavaScript String Extraction #26

Conversation

swissspidy commented Apr 27, 2018 • edited Loading

aduth commented Apr 30, 2018

aduth commented Apr 30, 2018

swissspidy commented May 9, 2018

swissspidy commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlessera commented Jul 4, 2018

swissspidy commented Apr 27, 2018 •

edited

Loading