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

Fix ts extract support #187

Merged
merged 3 commits into from
Aug 7, 2018
Merged

Fix ts extract support #187

merged 3 commits into from
Aug 7, 2018

Conversation

IvanoAlvino
Copy link

Since version 2.3.7 ts extract support is broken, reason beeing the ecmaFeatures jsx set to true also in case of ts extension.
Added an if case to set this to false for ts, and leave it to true for jsx.

Since version 2.3.7 ts extract support is broken, reason beeing the ecmaFeatures jsx set to true also in case
of ts extension.
Added an if case to set this to false for ts, and leave it to true for jsx.
@rubenv
Copy link
Owner

rubenv commented Aug 7, 2018

Do you have an example of a file that breaks? We have unit tests that verify a plain .ts file, so things should work.

@rubenv
Copy link
Owner

rubenv commented Aug 7, 2018

For example: https://github.com/rubenv/angular-gettext-tools/blob/master/test/fixtures/ts.ts

Tested here:

it('supports TypeScript files', function () {
var files = [
'test/fixtures/ts.ts'
];
var catalog = testExtract(files);
assert.equal(catalog.items.length, 2);
assert.equal(catalog.items[0].msgid, 'Hello');
assert.equal(catalog.items[0].msgstr, '');
assert.deepEqual(catalog.items[0].references, ['test/fixtures/ts.ts:2']);
assert.equal(catalog.items[1].msgid, 'One\nTwo\nThree');
assert.equal(catalog.items[1].msgstr, '');
assert.deepEqual(catalog.items[1].references, ['test/fixtures/ts.ts:3']);
});

@IvanoAlvino
Copy link
Author

This fixes the following reported error #181
We experienced the same problem, and we are locally able to fix it by setting jsx = false for ts files.

Can you please try with the following file?

angular.module("myApp").controller("helloController", (gettext) => {
	const castedVar: any = <any> gettext("Hello");
	gettext(); // Should be ignored.
});

@rubenv
Copy link
Owner

rubenv commented Aug 7, 2018

Yes, that seems to be the problem. Could you please add that to the test fixture and unit test? That way we're sure it never breaks again.

Also, instead of adding an extra if/else case, you could just write this:

            if (extension === 'ts' || extension === 'tsx') {
                syntax = tsParser.parse(src, {
                    sourceType: 'module',
                    comment: true,
                    ecmaFeatures: {
                        jsx: extension === 'tsx'
                    }
                });
            } else {

Thanks for submitting this!

@IvanoAlvino
Copy link
Author

Sure sounds good, fixes are coming :)

@IvanoAlvino IvanoAlvino force-pushed the fix-ts-parsing branch 2 times, most recently from d5b2388 to bbd43a3 Compare August 7, 2018 09:14
In addition, refactor a bit in order to remove an if/else construct
@rubenv
Copy link
Owner

rubenv commented Aug 7, 2018

Excellent fix, can go in now that we've got a test case.

@rubenv rubenv merged commit 3ce45d3 into rubenv:master Aug 7, 2018
@IvanoAlvino
Copy link
Author

Thanks for being very quick about this! :)
Have a nice day!

@IvanoAlvino IvanoAlvino deleted the fix-ts-parsing branch August 7, 2018 09:50
@rubenv
Copy link
Owner

rubenv commented Aug 7, 2018

Released to NPM as v2.3.12. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants