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(js/ts) fix detecting types as JSX #3278

Merged
merged 9 commits into from
Oct 17, 2021

Conversation

joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Jul 16, 2021

Resolves #3276.

Changes

  • adds a fallback default so when a confusing <....> block appears not to be JSX/HTML it's falls back to simply being ignored (by the JSX rule)
  • this should prevent false positives of JSX blocks that effectively (and in some cases silently) break further highlighting - because they open but never close

Prior there was no fallback so if the common cases didn't classify the block it would DEFAULT to being HTML... where-as we really shouldn't consider something HTML unless it not only LOOKS like HTML but we can also find a closing tag.

I'm not sure the first conditional is needed at all now and think perhaps:

  • If <blah is followed by > or space it MIGHT be HTML/JSX and we should search for a closing tag
  • if not, then the JSX rule should ignore it

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md

@joshgoebel joshgoebel requested a review from allejo July 22, 2021 23:55
@joshgoebel
Copy link
Member Author

I feel like this is ugly, but it does pass all test cases... ultimately this problem isn't really solvable (AFAIK) since we aren't a full contextual parser so I opted to do the simple thing that seems to fix the immediate reported issue.

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.

👍 for working with nested types

image

let m;
const afterMatch = match.input.substr(afterMatchIndex)
// NOTE: This is ugh, but added specifically for https://github.com/highlightjs/highlight.js/issues/3276
if (m = afterMatch.match(/^\s+extends\s+/)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (m = afterMatch.match(/^\s+extends\s+/)) {
if ((m = afterMatch.match(/^\s+extends\s+/))) {

This is just personal preference and I don't know if we have more of these in the codebase, but I usually surround assignments in if statements with an extra set of () so that it signifies it's not a type for ==.

Copy link
Member Author

@joshgoebel joshgoebel Jul 29, 2021

Choose a reason for hiding this comment

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

Ugh, why is From and string there highlighted diff in your snap? Grr, it's HTML...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, and if we don't flag it as HTML then we fall into the class "X extends Y" rule, which isn't really correct either... this may need more thought.

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.

(TypeScript) Generics with extends broken from 10.3.0
2 participants