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(javascript) comma is allowed in a "value container" #2403

Merged
merged 2 commits into from Feb 17, 2020

Conversation

joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Feb 16, 2020

  • fixes case where a regex would not be detected if it was anything
    other than the first parameter of a function call
  • in some cases this could actually cause the whole snippet to
    be flagged as illegal if the regex contained characters that
    were invalid at the top level (such as #)

This complexity is because we only detect regexs inside "value
containers" to prevent false positivies.

This issue was found when asking Highlight.js to highlight it's
own non-minified 1.2mb browser build.

Example of failing code (this was flagged illegal):

hljs.COMMENT(/\{#/, /#}/),

- fixes case where a regex would not be detected if it was anything
  other than the first parameter of a function call
- in some cases this could actually cause the whole snippet to
  be flagged as illegal if the regex contained characters that
  were invalid at the top level (such as #)

This complexity is because we only detect regexs inside "value
containers" to prevent false positivies.

This issue was found when asking Highlight.js to highlight it's
own non-minified 1.2mb browser build.
Copy link
Member

@allejo allejo left a comment

Choose a reason for hiding this comment

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

LGTM 👍 this works correctly even when there's a comma in the regex too.

hljs.COMMENT(/,\{#/, /,#}/),

@joshgoebel
Copy link
Member Author

Yeah, our regex regex should be decently good... the problem was just that it wasn't expecting the regex at all so then it would see the # and blow up...

I'm pretty impressed that it could highlight the whole 1.2mb source of itself without choking on anything else...

@joshgoebel
Copy link
Member Author

LGTM 👍 this works correctly even when there's a comma in the regex too.

If you're going to approve please actually click the Review/Approve button so it's counted as an official review so it'll trigger the ability to merge. :)

@allejo
Copy link
Member

allejo commented Feb 17, 2020

If you're going to approve please actually click the Review/Approve button so it's counted as an official review so it'll trigger the ability to merge. :)

I did approve, that's how it appears for me 😅

@joshgoebel
Copy link
Member Author

Now it's approved. :) Thanks.

@joshgoebel joshgoebel removed the request for review from egor-rogov February 17, 2020 02:44
@joshgoebel
Copy link
Member Author

Hmmm, seems Github doesn't like it much when you remove reviewers, lol.

@allejo
Copy link
Member

allejo commented Feb 17, 2020

I think it's because I approved as a triager, but GitHub wants an approval from someone with commit access. See how my approvals show up grey instead of green?

@joshgoebel
Copy link
Member Author

I think it's because I approved as a triager, but GitHub wants an approval from someone with commit access. See how my approvals show up grey instead of green?

Ah, hmmm...

@joshgoebel joshgoebel merged commit 4bc39be into highlightjs:master Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants