sourceLocations is broken when tags are implied

[thewilkybarkid]

This is deliberately left as a draft as it should demonstrate a bug as a result of fb55/htmlparser2#896, which causes parcel-bundler/parcel#6672.

I don't think it's possible to work around it here, but there will probably need to be code changes when the htmlparser2 bug is resolved.

[thewilkybarkid]

htmlparser2 has had a code change that looks to correct the issue there (fb55/htmlparser2#910), but the problem is still seen here (as the same indices are seen twice).

[thewilkybarkid]

Mostly works, but the start of the </p> paragraph seems to be calculated as the / rather than the <.

[thewilkybarkid]

Thanks, @fb55; unfortunately that change doesn't seem to make a difference...

[fb55]

unfortunately that change doesn't seem to make a difference

The start of a closing tag is now set to the < instead of the /. Do you have an example where it doesn't work?

[thewilkybarkid]

Unfortunately, CI isn't running on this PR yet, but the test fails on the implied start tag for the </p>:

      {
        location: {
          end: Object { … },
          start: {
  -         column: 19,
  +         column: 18,
            line: 2,
          },
        },
        tag: 'p',
      },

with and without the change. (18 being the expected value.)

Edit: Debugging it, the startIndex is 32 and the endIndex is 34 on the implied start tag. The former should be 31?

[fb55]

Good find! Turns out the index was overwritten for implied opening tags. I've opened fb55/htmlparser2#917 with a fix.

[thewilkybarkid]

@fb55 Thanks, the test's now passing. 😃

[thewilkybarkid]

I'll mark this as ready when you're able to do a new patch release @fb55.

[fb55]

I just published [email protected]. With this version you'll be able to check an isImplied flag on onopentag/onclosetag events, which should make this PR much simpler: fb55/htmlparser2#930

[thewilkybarkid]

That's great, thanks!

[thewilkybarkid]

Marking this as ready as there's a stable htmlparser release that's working.

Had a quick go at using the isImplied options and it doesn't seem to work as expected, the </b> in the test seems to flagged as implied, when it's not. /cc @fb55

[fb55]

Had a quick go at using the isImplied options and it doesn't seem to work as expected

Thanks for flagging! I published 7.1.1 with a fix for this, as well as for text events having wrong indices.

[thewilkybarkid]

Thanks, @fb55. It's now passing, and it is definitely simpler. ✨

[Scrum]

@thewilkybarkid @fb55 Thanks guys, released in version 0.10.1