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 parsing of `.js.map` files to improve reliability of JS translations #103

Merged
merged 3 commits into from Oct 31, 2018

Conversation

3 participants
@herregroen
Copy link
Contributor

herregroen commented Oct 30, 2018

Adds a new MapCodeExtractor to parse .map files ( that may accompany minified JS files ).

It will parse these using the existing JsFunctionsScanner to improve picking up translations and translator's comments that may get lost during build steps and/or minification.

WP_CLI::debug(
sprintf(
'Could not parse file %1$s: %2$s (line %3$d, column %4$d)',
$options['file'],

This comment has been minimized.

@schlessera

schlessera Oct 31, 2018

Member

This will point to the .js file, not the .js.map file, due to the change in line 31. That seems misleading in this case, as the source map failed to parse, not the .js file. The .js file would probably be a "webpack-ed" JS file, and the line and column references will be completely off.

I think the error message itself might need a clarification, as the line & column probably don't even exist in an actual file in that same location.

@schlessera

This comment has been minimized.

Copy link
Member

schlessera commented Oct 31, 2018

What about conflicts in comments between the different files. I guess the .js file and the .js.map file would either contain the exact same comment (in which case nothing should happen) or the .js file has no comment at all, in which the one in the .js.map file should be used.

Would there be other scenarios we need to take into account here?

@herregroen

This comment has been minimized.

Copy link
Contributor

herregroen commented Oct 31, 2018

What about conflicts in comments between the different files. I guess the .js file and the .js.map file would either contain the exact same comment (in which case nothing should happen) or the .js file has no comment at all, in which the one in the .js.map file should be used.

Would there be other scenarios we need to take into account here?

I think at the core this isn't so much a problem in parsing map files but rather a more generic one.

What happens if I have the following PHP file?

// translators: %s is the name of the product.
__( "Add %s", "my-domain" );

// translators: %s is the name of the post type.
__( "Add %s", "my-domain" );

Which, in current behaviour would add both comments:

public function addExtractedComment($comment)
    {
        if (!in_array($comment, $this->extractedComments, true)) {
            $this->extractedComments[] = $comment;
        }

        return $this;
    }

I think that behaviour is fine for map files as well. If the comment is already there then don't add it. If it's not then add it. If there happen to be two different comments there's really no way to automatically determine which one is more correct so the most sensible thing is to add both.

@schlessera schlessera added this to the 2.0.3 milestone Oct 31, 2018

@schlessera schlessera merged commit d528b0d into wp-cli:master Oct 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schlessera schlessera changed the title Add parsing of .map files to improve reliability of JS translations Add parsing of `.js.map` files to improve reliability of JS translations Oct 31, 2018

@swissspidy

This comment has been minimized.

Copy link
Collaborator

swissspidy commented Oct 31, 2018

What happens if I have the following PHP file?
Which, in current behaviour would add both comments:

The audit function will print a warning in this case because the string has two different translator comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment