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

Improved parsing of Trans component #85

Merged
merged 9 commits into from
Apr 7, 2018
Merged

Conversation

coyotte508
Copy link
Contributor

@coyotte508 coyotte508 commented Apr 2, 2018

Based on https://react.i18next.com/components/trans-component.html

Each html tag (closing or self-closing) and each js expression is converted to <x>content</x> where x is the index of the tag/expression in the array of children of the current tag (Trans being the root tag).

The example:

Hello <strong title={t('fourth')}>{{name}}</strong>, you have {{count}} unread message. <Link to="/msgs">Go to messages</Link>.

Becomes:

Hello <1><0>{{name}}</0></1>, you have <3>{{count}}</3> unread message. <5>Go to messages</5>.

You may notice that I deviated from the usual way of parsing. I use regexps for parsing quotes, but for the rest I don't use regexps. This avoids some caveats with regexps, for example <a stuff="a>" ....> with regexps the first closing > would trigger the end of the tag even though it shouldn't as it's surrounded by quotes.

The js parser could be improved to try and detect keys of objects, for example {{name: 'Albert'}} is now parsed as is, but it should render {{name}}. That said, it's good practice to set the variables before rendering the jsx, and as long as the user of the parser is aware, there should be no problems.

@karellm
Copy link
Member

karellm commented Apr 2, 2018

Thanks for the PR, I will review it later today or tomorrow

@coyotte508
Copy link
Contributor Author

coyotte508 commented Apr 4, 2018

Alright, let me know if there's anything I can do to make it easier

Copy link
Member

@karellm karellm left a comment

Choose a reason for hiding this comment

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

I realize that some of my comments are based on an undocumented style guide. At some point I will add a linter to avoid that.

My main concern really is the lack of test for the methods of the lexer. And since it adds a lot of complexity, I would vote for using an existing library.

* @param {*} string
*/
parseJsx(string) {
if (string.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

The code usually uses !string for this kind of check

Copy link
Contributor Author

@coyotte508 coyotte508 Apr 6, 2018

Choose a reason for hiding this comment

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

The problem is that it would not parse '0' correctly in that case. As '0' == false.

Copy link
Member

@karellm karellm Apr 6, 2018

Choose a reason for hiding this comment

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

Well !'0' is false and '0'.length === 0 is also false. Only empty string is a falsy value (aka length == 0)

*
* @returns string
*/
eraseTags(string) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please test these methods in the lexer's test? The parser test is a high level check but individual method that do as much as these one should have test of their own so we can debug them later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test for eraseTags.


const tag = /[A-Z0-9-]+/i.exec(string)[0].toLowerCase()

let currentIndex = tag.length+1
Copy link
Member

Choose a reason for hiding this comment

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

Please add space around operators tag.length + 1


let currentIndex = tag.length+1

while (currentIndex < string.length) {
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered a html parser rather than implementing this yourself? There are a bunch listed here.

If we want code so complex in this package, I would require a lot more tests for edge cases. I really think relying on a library is the right way to go here.

After a quick search, there seems to be a library just for react here based on htmlparser2.

Copy link
Contributor Author

@coyotte508 coyotte508 Apr 6, 2018

Choose a reason for hiding this comment

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

I don't have any issue with using an existing html parser. I'll look into it.

@coyotte508
Copy link
Contributor Author

@karellm It's done, using acorn-jsx: https://github.com/RReverser/acorn-jsx.

None of the other parsers I found, including react-html-parser and html-react-parser, htmlparser2, parse5, could handle js expressions mixed in html.

Copy link
Member

@karellm karellm 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 quick update. I'm glad that there was a library that worked in the end. I added couple more comments but I'm also happy to take over for minor style changes.

* @param {string} originalString The original string being parsed
*/
simplify(children, originalString) {
for (let i = 0; i < children.length; i ++) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you don't use forEach here? If not, it would be more consistent with the rest of the code

* @returns string
*/
eraseTags(string) {
const children = this.simplify(acorn.parse(string, {plugins: {jsx: true}}).body[0].expression.children, string);
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid line that are longer than 80 char. I would suggest doing this in two steps

eraseTags(string) {
const children = this.simplify(acorn.parse(string, {plugins: {jsx: true}}).body[0].expression.children, string);

const elemsToString = children => children.map((child, index) => {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you create a function rather than directly returning the mapped children?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it calls itself recurisvely.

* @param {*} children An array of elements contained inside an html tag
* @param {string} originalString The original string being parsed
*/
simplify(children, originalString) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you name this something a little more idiomatic like parseAcornPayload?

// Filter empty text elements. Using string.length instead of !string because
// '0' is a valid text element, and '' is not, and !string doesn't make a difference
// between the two.
children = children.filter(child => !(child.type === 'text' && child.content.length === 0));
Copy link
Member

Choose a reason for hiding this comment

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

This could be written child.type !== 'text' || child.content which is easier to read imo

I'm not sure about your comment either. As I commented above: '0' is truthy, '' is falsy. They are not the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, '0' == false, yet !'0' == false too... A quirk of js

@coyotte508
Copy link
Contributor Author

Thank you for your fast review. I just pushed some changes, don't hesitate to take over if needed.

@karellm karellm merged commit 9ef46f5 into i18next:master Apr 7, 2018
@karellm
Copy link
Member

karellm commented Apr 7, 2018

Thanks for the contribution, 1.0.0-beta9 was just released

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.

None yet

2 participants