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

Add support for indirect translation function calls in JS #228

Merged
merged 8 commits into from
Mar 5, 2021

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Sep 16, 2020

This change allows makepot on JS files to work for babel transpiled files. This change is similar to #204 but related to a babel behavior instead.

In some cases, babel will convert simple function calls to indirect ones (in the form of (0, __)( 'translate' )).

I guess this is done so that legacy ES5 script that uses strict mode are able retrieve the global window object using this.

In any case, babel often transpiles code like:

import { __ } from '@wordpress/i18n';

__( 'translate' );

Into:

var _i18n = _$$_REQUIRE(_dependencyMap[4], "@wordpress/i18n");

(0, _i18n.__)( 'translate' );

Testing instructions

@Tug Tug requested a review from a team as a code owner September 16, 2020 09:57
@swissspidy
Copy link
Member

Really nice, thank you!

@Tug Tug force-pushed the update/js-function-scanner-babel branch from abdde37 to ab8d287 Compare September 16, 2020 10:55
@Tug
Copy link
Contributor Author

Tug commented Sep 16, 2020

Thanks for the review @swissspidy

I just added a last minute check to exclude a edge case: (0, __, somethingElse)( 'translate' )

@swissspidy
Copy link
Member

No idea why the tests are timing out 😕

@Tug Tug force-pushed the update/js-function-scanner-babel branch from 4bfdcfe to 3cf37af Compare September 23, 2020 17:53
@Tug
Copy link
Contributor Author

Tug commented Sep 23, 2020

Just force pushed to give Travis another chance.

@Tug
Copy link
Contributor Author

Tug commented Sep 29, 2020

Seems like the test jobs for ubuntu xenial with php 5.6 are already close to 10min on the master branch, so it's possible we're reaching travis limit just by adding those few tests. Just experimenting with travis config a bit here ^

@swissspidy
Copy link
Member

@schlessera any thoughts here?

@Tug
Copy link
Contributor Author

Tug commented Sep 29, 2020

It's green now, and it looked like those tests indeed needed a couple more minutes to finish. Happy to leave 5b9d70e in if you think it's the right approach

@swissspidy
Copy link
Member

Interesting! Thanks for debugging.

I think for the sake of consistency with all other repos we should undo the travis_wait part again. But maybe Alain has some thoughts here.

Copy link
Member

@schlessera schlessera left a comment

Choose a reason for hiding this comment

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

As @swissspidy already mentioned, the travis.yml file is supposed to be shared across all repos, so I'd like to find a solution that doesn't require changes to that.

The problem seems to be with PHP 5.6 only, so I assume there's something being used that has an extension on PHP 7+ but not on PHP 5.6.

So we'd need to find out what it is that is so much slower.
If there's an extension we can install to accelerate PHP 5.6 builds, I'm happy to add it centrally to all repos.
If not, we can skip the most expensive tests by strategically marking some tests as @require-php-7-0.

src/JsFunctionsScanner.php Outdated Show resolved Hide resolved
src/JsFunctionsScanner.php Outdated Show resolved Hide resolved
src/JsFunctionsScanner.php Outdated Show resolved Hide resolved
@schlessera
Copy link
Member

Hmm, if Travis times out after 10 minutes on Behat, this means Behat didn't produce a single . for more than 10 minutes...

So there seems to be a single operation in the tests somewhere that runs for a very long time...

@schlessera
Copy link
Member

I'll try to add a speed listener of some sort to Behat, reporting on the slow(est) scenarios and/or steps.

@simison
Copy link

simison commented Jan 27, 2021

Hi folks! Anything in particular stopping this PR from proceeding?

@swissspidy
Copy link
Member

Hi folks! Anything in particular stopping this PR from proceeding?

I think mainly the issue with the tests timeout needs to be addressed.

@schlessera schlessera force-pushed the update/js-function-scanner-babel branch from f842278 to f0b94af Compare March 5, 2021 09:34
@schlessera schlessera force-pushed the update/js-function-scanner-babel branch from f0b94af to 74f39c8 Compare March 5, 2021 09:52
@schlessera schlessera added this to the 2.2.7 milestone Mar 5, 2021
@schlessera schlessera merged commit 606bbab into wp-cli:master Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants