JS - change CLASS_REFERENCE regex to also accept numbers and underscore

[rbrishabh]

fixes #3252

Hey @joshgoebel! How does this look?
Let me know if this needs any changes! Thanks

[joshgoebel]

Adding a markup test for this case totest/markup/javascript/class would be a good idea.

[rbrishabh]

What do you think? @joshgoebel
Thank you for your quick reviews!

[fserb]

I appreciate the quick fix, but is /\b[A-Z][a-z0-9_]+([A-Z][a-z0-9_]+)*/ correct?

There are a few problems that I can see here:

• there's this assumption that all classes must start with an uppercase, so new myClass() won't work. Is this on purpose?
• it's also assuming that it must be followed by a lowercase. So new CSSThing() won't work.
• it's also missing $, which is a valid character for JS class name (think JQuery's $), but really: new my$c is valid JS.
• it's also missing $ and _ as valid initial characters. new _X() or new $X(). For the $ you probably need to remove the \b.
• Unless I'm missing something about grouping, the first + is wrong and the (....+)* could be replaced by *.

I think those points above could be addressed with something like /[a-zA-Z\$_][a-zA-Z\$_0-9]*/

It's also missing unicodes, but this may be a bit beyond the scope of this change. This stack overflow answer has a complete solution, but not what highlight.js needs, because builtins seem to be cleared up before).

[joshgoebel]

there's this assumption that all classes must start with an uppercase, so new myClass() won't work. Is this on purpose?

This is a conventions rule, not an absolute one. Uppercase classes is a hugely common convention, but a lowercase symbol could be anything. That said if someone wanted to add a mode specifically for new blah, that would be ok. But this rule cannot be expanded because we'd have too many false positively.

it's also assuming that it must be followed by a lowercase. So new CSSThing() won't work.

This should indeed be taken into account. So we need a few more markup tests.

it's also missing $, which is a valid character for JS class name (think JQuery's $), but really: new my$c is valid JS.

This seems an edge case and same problem as first point, false positives.

it's also missing $ and _ as valid initial characters. new _X() or new $X(). For the $ you probably need to remove the \b.

Ditto.

Unless I'm missing something about grouping, the first + is wrong and the (....+)* could be replaced by *.

Good point, but the whole thing may need to be slightly rethought now to handle cases like CSSHandler... so

[joshgoebel]

@rbrishabh You need to actually run the test suite, and update the test expectations also, etc... see our docs for building and testing. Now we also need to add test for class names like ASTParser... Perhaps something like [A-Z][A-Za-z0-9_]+ ?

[rbrishabh]

Sure! I can look into this later today!

[rbrishabh]

hey @joshgoebel! How does this look now?

[fserb]

This is okey, although technically it should be * and not + (otherwise new X() won't work). But I'm not sure if this breaks some other logic on HighlightJS.

[joshgoebel]

I think one letter class names is an edge case. We really want to see the camel case pattern to trigger this heuristic.

[joshgoebel]

Let's focus more on the tests... at the very end I might take one last look and tweak this a variety of diff ways.

[joshgoebel]

I think we need BOTH... I played with this briefly.

CRV with zero context could be a constant (all CAPS is a constant) or a class name... Changing this rule as it is now incorrectly identifies constants as classes (or vice versa)... I think perhaps we need to handle both:

• CRV (constant)
• CRV( (class)

It's not perfect, but we can't get perfect since these are conventions, not language rules. So we'd use the camel case rule (with zero other context) PLUS the newer new rule proposed here, but only if a look-ahead sees a (. I think that would cover a lot of cases. Thoughts?

[joshgoebel]

I'm still looking for an updated test/markup/javascript/class.expect.txt but don't see it... if you want to let HLJS update the tests for you (and confirm them by inspection) there is a line you can uncomment in test/markup/index to do this. (just uncomment it and run the markup tests once to write out the new expectations)

[joshgoebel]

Ping.

[rbrishabh]

hi! I have a busy schedule and cant keep up with this now! Please feel free to continue it from here. I can close it for now

[joshgoebel]

Thanks anyways! It'll be here if anyone needs to reference it when there is time to pick this back up.