The upstream issue is fixed, but a bunch of tests for <Trans /> fail now. Example:

FAIL  test/trans.render.list.spec.js (28.115s)
  ● Trans should render nested components › should render dynamic ul as components property

    expect(received).toMatchInlineSnapshot(snapshot)

    Snapshot name: `Trans should render nested components should render dynamic ul as components property 1`

    - Snapshot
    + Received

    - <div>
    -   Result should be a list: 
    -   <ul>
    -     <li>
    -       li1
    -     </li>
    -     <li>
    -       li2
    -     </li>
    -   </ul>
    - </div>
    + <div />

Any thoughts?

PS: If it’s easier to fix this than to explain me where to dig, happy for my commits to be recycled in another PR 🙂

Practically all Trans tests are failing...
I suspect the generated ast looks completely different.... https://github.com/i18next/react-i18next/blob/master/src/Trans.js#L136

rayd/html-parse-stringify2#27

@adrai thanks for your alternative solution! To increase our chances for a fast resolution, do you know what I could change in this PR for the tests to pass? I haven’t used html-parse-stringify or html-parse-stringify2 before, so am not familiar with their differences or expectations. Intuitively, it feels better to return back to html-parse-stringify from the fork.

@kachkaev guess you have to write new snapshots -> and manually check first if received output still matches expected and the changes are pure formatting and not wrong html

Hmmm here’s the catch. When we call:

const ast = HTML.parse(`<0>${interpolatedString}</0>`);

using html-parse-stringify@2.0.3, the result is an empty array. This is because of:

const tagRE = /<[a-zA-Z\-\!\/](?:"[^"]*"|'[^']*'|[^'">])*>/g

Thus, tags like <0>, <1> etc. are ignored.

If rayd/html-parse-stringify2#27 is merged as is, we’ll end up with the same problem in html-parse-stringify2 too.

PR rayd/html-parse-stringify2#27 is adapted

Two blockers on the path to html-parse-stringify:

• Support numeric tags HenrikJoreteg/html-parse-stringify#43
• Add broken tests for multiple nested tags HenrikJoreteg/html-parse-stringify#44

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

bump

@HenrikJoreteg has just released html-parse-stringify@2.1.0, which is great! The new version includes HenrikJoreteg/html-parse-stringify#43 so we are half-way there!

Once HenrikJoreteg/html-parse-stringify#41 is merged and released, all tests should be passing and we’ll be able to switch from html-parse-stringify2 to html-parse-stringify 👍

I went into my local nodes_modules and manually tweaked html-parse-stringify/dist code so that it included HenrikJoreteg/html-parse-stringify#41. Interestingly, I noticed that some snapshots still ended up failing, so I updated them. See diff.

Is this a regression or just an artifact in snapshots? 🤔 If it is a regression, is it something to fix here or upstream? Potentially, we might have a lost space between </closingTag> <openingTag>.

Is this a regression or just an artifact in snapshots? 🤔 If it is a regression, is it something to fix here or upstream? Potentially, we might have a lost space between </closingTag> <openingTag>.

No errors on master:

No errors on master is an expected thing, because in there we’ve got "html-parse-stringify2": "^2.0.1". This PR replaces that dependency with html-parse-stringify, which either has a slightly different shape of an AST or misses a space in an AST in the first place. I don’t have a solution on top of my head right now, so if anyone could do some extra digging, that’d be great!

Where I’m going is that we might need to either submit an extra patch to html-parse-stringify in addition to HenrikJoreteg/html-parse-stringify#41 or maybe amend AST post-processing here.

rayd/html-parse-stringify2#27 is still a good alternative solution and I’m happy with any that can land first. The main goal here is to close #1275 🙂

Coverage remained the same at 96.205% when pulling c85d72a on kachkaev:cve-2021-23346 into 4c005be on i18next:master.

Thanks to @HenrikJoreteg’s work on merging HenrikJoreteg/html-parse-stringify#41 and releasing of 2.1.1, CI is finally green now! However, for the tests to work I had to remove extra new lines from the snapshots. We need to investigate this.

My gut feeling is that there’s another small bug in html-parse-stringify, in particular to do with treating </a> <b> as </a><b>. The alternative cause could be related to AST handling in react-i18next (which would be easier to fix).

To confirm or deny the above hypotheses we’ll need to add some extra test cases upstream. I’m not sure when exactly I’ll be able to do this, so if anyone wants to help, feel free to 👍

We are much closer to publishing this PR than we were a couple of days ago, so thanks to everyone who is involved 🙌

@HenrikJoreteg I had a quick look at the generated ast of this failing test: https://github.com/i18next/react-i18next/blob/master/test/trans.render.spec.js#L129

Just a simple console.log(JSON.stringify(ast, null, 2)) here: https://github.com/i18next/react-i18next/blob/master/src/Trans.js#L137

The output of html-parse-stringify differs from html-parse-stringify2 in the following way:

This text element is missing in html-parse-stringify before the br element:

      {
        "type": "text",
        "content": " "
      }

html-parse-stringify2:

[
    {
        "type": "tag",
        "name": "0",
        "voidElement": false,
        "attrs": {},
        "children": [
            {
                "type": "tag",
                "name": "strong",
                "voidElement": false,
                "attrs": {},
                "children": [
                    {
                        "type": "text",
                        "content": "Go"
                    }
                ]
            },
            {
                "type": "text",
                "content": " "
            },
            {
                "type": "tag",
                "name": "br",
                "voidElement": true,
                "attrs": {},
                "children": []
            },
            {
                "type": "tag",
                "name": "1",
                "voidElement": false,
                "attrs": {},
                "children": [
                    {
                        "type": "text",
                        "content": "there"
                    }
                ]
            },
            {
                "type": "text",
                "content": "."
            }
        ]
    }
]

html-parse-stringify:

[
    {
        "type": "tag",
        "name": "0",
        "voidElement": false,
        "attrs": {},
        "children": [
            {
                "type": "tag",
                "name": "strong",
                "voidElement": false,
                "attrs": {},
                "children": [
                    {
                        "type": "text",
                        "content": "Go"
                    }
                ]
            },
            {
                "type": "tag",
                "name": "br",
                "voidElement": true,
                "attrs": {},
                "children": []
            },
            {
                "type": "tag",
                "name": "1",
                "voidElement": false,
                "attrs": {},
                "children": [
                    {
                        "type": "text",
                        "content": "there"
                    }
                ]
            },
            {
                "type": "text",
                "content": "."
            }
        ]
    }
]

Hi @pavanjava! The goal of this PR is to mitigate CVE-2021-23346 – you are right about it. The ETA for a new react-i18next release depends on how fast we will resolve this bug in html-parse-stringify: #1283 (comment). Feel free to help!

Tried to create a PR: HenrikJoreteg/html-parse-stringify#45
Really not sure if this is the correct way to do so...
this in combination with passing { respectWhitespace: true } in react-i18next:

could fix the tests...
But honestly I do not even know if having or not having that whitespace matters?

Tried to create a PR

Great stuff, thanks @adrai!

I do not even know if having or not having that whitespace matters?

I guess it might matter. If we have <b>hello</b> <i>world</i> and the whitespace token is removed, we end up with helloworld.

any news?

I think I'm the hangup, i'm trying to sort out if it should be implemented as an option (as was done) in the PR or if it should just be the normal behavior to always preserve it. I'd rather not make it an option unless we can think of a reason why anyone would want to not have the whitespace.

I second an option that avoids having options. If anyone wants to not have space between two tags, they can remove it in the source string 🙂

I second an option that avoids having options. If anyone wants to not have space between two tags, they can remove it in the source string 🙂

new major version of html-parse-stringify ;-) HenrikJoreteg/html-parse-stringify#45 (comment)

Good news: HenrikJoreteg/html-parse-stringify#46 (comment)

@kachkaev you can choose:

1. Do you want to update this PR?
2. I'll do it directly on master and create a new version?

Great news! 🎉 Happy to update the PR shortly 🙌

patch?

🎉🎉🎉🎉🎉🎉 PUBLISHED v11.8.13 🎉🎉🎉🎉🎉🎉🎉