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 as a code owner Sep 16, 2020
@swissspidy
Copy link
Member

@swissspidy swissspidy commented Sep 16, 2020

Really nice, thank you!

Loading

features/makepot.feature Show resolved Hide resolved
Loading
@Tug Tug force-pushed the update/js-function-scanner-babel branch from abdde37 to ab8d287 Sep 16, 2020
@Tug
Copy link
Contributor Author

@Tug 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' )

Loading

@swissspidy swissspidy requested a review from schlessera Sep 19, 2020
@swissspidy
Copy link
Member

@swissspidy swissspidy commented Sep 19, 2020

No idea why the tests are timing out 😕

Loading

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

@Tug Tug commented Sep 23, 2020

Just force pushed to give Travis another chance.

Loading

@Tug
Copy link
Contributor Author

@Tug 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 ^

Loading

@swissspidy
Copy link
Member

@swissspidy swissspidy commented Sep 29, 2020

@schlessera any thoughts here?

Loading

@Tug
Copy link
Contributor Author

@Tug 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

Loading

@swissspidy
Copy link
Member

@swissspidy swissspidy commented Sep 29, 2020

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.

Loading

Copy link
Member

@schlessera schlessera left a comment

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.

Loading

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

@schlessera schlessera commented Sep 29, 2020

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...

Loading

@schlessera
Copy link
Member

@schlessera schlessera commented Oct 1, 2020

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

Loading

@simison
Copy link

@simison simison commented Jan 27, 2021

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

Loading

@swissspidy
Copy link
Member

@swissspidy swissspidy commented Jan 27, 2021

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

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

Loading

@schlessera schlessera force-pushed the update/js-function-scanner-babel branch from f842278 to f0b94af Mar 5, 2021
@schlessera schlessera force-pushed the update/js-function-scanner-babel branch from f0b94af to 74f39c8 Mar 5, 2021
@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
24 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants