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

Feat/parse html from js template string #64

Conversation

JeffersonCarvalh0
Copy link
Contributor

I was facing a very similar problem to the one related in #21 so I decided to give it a try. The idea is to create a JS extractor that accepts an HTML parser as a parameter, in order to parse the HTML from template strings (pretty much the same idea from the embeddedJs extractor).
Please, let me know how I can improve this PR, and thanks for this amazing library!

PS: I'm not sure the linter worked

@lukasgeiter
Copy link
Owner

Thanks for your PR!

I'm wondering if limiting this feature to only template string literals with no substitutions is maybe a bit too restrictive? What are your thoughts on that?

@JeffersonCarvalh0
Copy link
Contributor Author

I agree with you @lukasgeiter , I'm just not sure about which types I could check for (there are barely any docs for the compiler API from typescript - if you found a helpful source somewhere, please I'd like to know)

I could test with other types I find fitting in the .d.ts files from typescript and add them to this feature

@lukasgeiter
Copy link
Owner

No worries. I recommend you use a tool that makes it easy to explore the AST. Here's two I've used before:

For this feature there are two potential types of string literals to handle. First, regular string literals (double and single quotes). This one is quite straightforward: SyntaxKind.StringLiteral. Even better, use isStringLiteralLike to check for string literals and template strings without substitutions at once.

Secondly, there's the topic of template strings with substitutions. This one will be a bit more complex because you would first have to build a full string out of the parts that can then be passed to the html parser. Before you attempt that, I would be curious if you have this case in your code base. Just to get an idea if it's even worth it to implement support for template strings with substitutions.

@JeffersonCarvalh0
Copy link
Contributor Author

Thanks for the tips!

As for html templates within template strings with substitutions, if I understood correctly, I actually have them in the code base, but they are already being extracted. My guess is that the JS extractor actually "sees" the JS code inside the template because it is already programmed to do that, regardless of the string surrounding it, so it doesn't matter if it is an HTML template (and the interpolated values don't generate more HTML code, they are literally calls to the i18n extraction function). I can't think about a use case where we'd have to write a specific logic to handle that

@JeffersonCarvalh0
Copy link
Contributor Author

I've added isStringLiteralLike as suggested and also added a new test case to illustrate the cases I have in my codebase where template substitutions are used (note that I've added this test to the callExpression tests, not the htmlTemplate tests)

@lukasgeiter
Copy link
Owner

Yes, expressions inside template strings are JavaScript so they are properly extracted. There's no need for an explicit test for that. Otherwise I would have to add tests for every possible place a call expression can occur in JavaScript.

What I'm talking about is running the HTML parser/extractor on a template string with substitutions. For example:

`<div>
    <translate>Hello World</translate>
    ${substitution}
</div>`

.npmignore Outdated Show resolved Hide resolved
tests/js/extractors/factories/htmlTemplate.test.ts Outdated Show resolved Hide resolved
@JeffersonCarvalh0
Copy link
Contributor Author

No worries. I recommend you use a tool that makes it easy to explore the AST. Here's two I've used before:

For this feature there are two potential types of string literals to handle. First, regular string literals (double and single quotes). This one is quite straightforward: SyntaxKind.StringLiteral. Even better, use isStringLiteralLike to check for string literals and template strings without substitutions at once.

Secondly, there's the topic of template strings with substitutions. This one will be a bit more complex because you would first have to build a full string out of the parts that can then be passed to the html parser. Before you attempt that, I would be curious if you have this case in your code base. Just to get an idea if it's even worth it to implement support for template strings with substitutions.

Sorry about the delay.

Now I got what you meant. I don't think I have this case in my codebase =/
I've just applied the changes you suggested

Copy link
Owner

@lukasgeiter lukasgeiter left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!
Let's not bother with templates that have substitutions then. Support can still be added later if requested.

tests/js/extractors/factories/callExpression.test.ts Outdated Show resolved Hide resolved
tests/js/extractors/factories/htmlTemplate.test.ts Outdated Show resolved Hide resolved
tests/js/extractors/factories/htmlTemplate.test.ts Outdated Show resolved Hide resolved
Copy link
Owner

@lukasgeiter lukasgeiter left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, and apologies for my delay. I think we're almost there 🙂

src/js/parser.ts Outdated Show resolved Hide resolved
tests/js/extractors/factories/htmlTemplate.test.ts Outdated Show resolved Hide resolved
tests/js/extractors/factories/htmlTemplate.test.ts Outdated Show resolved Hide resolved
@JeffersonCarvalh0
Copy link
Contributor Author

Sorry for the delay on my part, these last weeks have been busy =/
Please let me know if there is anything else that needs to be changed

@lukasgeiter lukasgeiter merged commit 4563ac0 into lukasgeiter:master Jul 10, 2023
5 checks passed
@lukasgeiter
Copy link
Owner

Released with v3.8.0

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