feat(handlebars): add subexpressions, block-params and hash-parameters

[nknapp]

closes #2181

• Support for sub-expressions: Helper names in sub-expressions are now highlighted properly. The mode-tree is built recursively for sub-expressions so that sub-elements (nested sub-expressions in params) are also highlighted properly (like {{helper1 (helper2 (helper3 param) ) }}).
• Support for hashes ("attr-assignments" in the "htmlbars"-language) (like {{helper key=value}} or {{helper key=(sub-helper value)}}
• Path expressions are highlighted: {{path.expression}}. Up to know, only path would be highlighted.
• Block parameters are highlighted correctly: In {{#with as as | as | }} , only the middle as is a keyword.
• Literals in helper-parameters are highlighted as keyword, number or string.
• Merged built-ins with the list from 'htmlbars'
• Added 'htmlbars' as alias
• Deleted the 'htmlbars' language

[nknapp]

@yyyc514 I think I am about ready except for some cleanup and the following questions:

• what should happen to 'htmlbars' now?
  I have removed the whole language in my branch because the auto-detection tests were failing all the time.
• should the changes go to the wip_version_10 branch?
• The expression inside I basic mustache ({{test}}) is now highlighted as "hljs-name" just like in block-mustaches. I felt that they should because they essentially have the same semantics. Should I revert that to retain the previous behaviour?

[joshgoebel]

what should happen to 'htmlbars' now?

I've changed my thinking on this and now think it should simply be an alias of handlbars. (particularly if we feel it really covers everything).

should the changes go to the wip_version_10 branch?

Sure, though I hope to merge it soon anyways, so that's not a necessity.

The expression inside I basic mustache ({{test}}) is now highlighted as "hljs-name" just like in block-mustaches. I felt that they should because they essentially have the same semantics. Should I revert that to retain the previous behaviour?

What was the previous behavior again?

[nknapp]

Before (on the current master), non-block mustache expressions are rendered like this:

After the PR like this:

Here is a complete overview of all testcases:

Before the PR:

After the PR:

[nknapp]

What I also changed (in my next version, is the attr-assignment-rule of htmlbars (in Handlebars-speak, this is called "Hash"). The 'htmlbars'-language highlights the value (behind the equal-sign) as attr, but as I understand it, the part in front of the equal sign should be highlighted.

This change is not visible in the default theme, because the "attr"-class has no special highlighting, but it is there.

[joshgoebel]

The expression inside I basic mustache ({{test}}) is now highlighted as "hljs-name" just like in block-mustaches.

I kind of dislike this for simple variable expressions, but I don't really know what to suggest as figuring out one from the other might not be worth the effort.

[joshgoebel]

but as I understand it, the part in front of the equal sign should be highlighted.

Makes sense to me, of course if the value was a number or string I'd still expect it to be highlighted as such.

[nknapp]

I kind of dislike this for simple variable expressions, but I don't really know what to suggest as figuring out one from the other might not be worth the effort.

The distinction between function-call and property-access is not between blocks and basic mustaches (see https://jsfiddle.net/nL91aohe/1/):

• {{test}} can be a property-access (i.e. variable) or a function-call (if 'test' is a function) or a helper call (if a helper 'test' is registered)
• {{#test}}...{{/test}} can be a block-helper, or a property-access that dives into a object.

But, I have left the classes "variable" and "tag" as they were. I just gave the first element in a a basic mustache the class "name", which makes it bold.

[joshgoebel]

I just gave the first element in a a basic mustache the class "name", which makes it bold.

Don't presume this, how it looks all depends on the theme used...

[nknapp]

This has gotten really complex now. I wonder if you'll be able to review...

It is really difficult to verify the tests manually. I tried to make them self-explanatory, but the HTML-tags in the expect-files really mess up with readability.

It might be a good idea to create use a visual regression-testing tool to create screenshots of the test-cases. Just an idea... Of course, we would need a theme that makes more classes visible...

I'll try to update the description of the PR with a summary of what has changed...

[nknapp]

What I am still unsure about is the class for built-in helpers and keywords:

As far as I understand the class reference it should be like this:

• built-in helpers like 'each', 'with' (and probably all the other keywords in the built-ins list as well) should get the class builtin-name because they are a "name from the language standard library" .
• keywords like {{else}} or as in {{#with x as | y |}} should get the class "keyword" because they are part of the language and not the standard library

What is the 'built_in' class used for? Should any of those be built_in instead?

[nknapp]

Apart from that, I am done here for now. I think you can start your review...

[joshgoebel]

I'd love a better way to write markup tests, but it seems a tough problem. The way we do it now is surely because we never know what might be a validate language so there is no safe way to escape anything inside the test file.... hence we end up with raw text or using comments to describe individual test cases. (but those can get in the way of seeing the test cases themselves)

Something like JSON/YAML would probably be preferable but then optimally we'd have to build out a whole test case editor... which I don't think anyone has the time for.

[nknapp]

I was just wondering if you are waiting for a answer to your last comment. But I didn't see a question, that's why I didn't write anything new.

I'm just doing it now, a friendly reminder that this is ready for review from my point of view.

[joshgoebel]

I asked a few questions/requested changes in the sub-responses.

[joshgoebel]

See my individual comments.

[nknapp]

Haven't seen the comment. Will look at it.

[nknapp]

@yyyc514
I have refactored the commits and some tests and rebased everything onto the master. The commits are now structured as follows:

1. Implementation of changes, adjustet test-cases expectations to reflect the new rules. This should allow you to see the changes that the new code does to the old tests
2. Add more tests for the new rules.
3. Change of the behaviour regarding {{else}} that you requested above.

I have not added any text parts to the new tests, except in the test "combinations-with-text.txt".

I have left the text parts in the old test-cases, because I didn't want to change the tests (make it easier for you to compare the results). I can clean-up those tests as well, if you want me to.

[nknapp]

Somehow, GitHub displays the commits in the wrong order. The correct order (in my git repo) is:

9468f00 wip: adjustments for integrating handlebars rules
30a5e54 fixup: add more tests
c6fbcec fixup: make only "else" in {{else}} the keywords, not "{{else}}" as whole

[nknapp]

@yyyc514 I have added a few commits that hopefully address all of your points: Some commits, I have also noticed and fixed the following things:

• Typo in variable IDENFIFIER_REGEX (commit 74f0c87 was somehow attributed to you, not me. I made an interactive rebase and moved that commit to the beginning and something went wrong. I don't mind if you don't).
• The jsdoc-comment in regex.concat was not accurate. IntelliJ gave me a warning, a varargs-
  parameter is not an array in jsdoc.

I think it would generally be better to use

import {concat} from '../lib/regex'

then

import * as regex from '../lib/regex' 

because then, rollup can optimize using tree-shaking. I haven't done it because the wildcard import was used in other places. This might be an additional PR, for other languages as well.

[joshgoebel]

Thanks for the type fix. :-) Your right.

---

I prefer seeing the regex namespace in the code. Though I suppose you could still import them one at a time then put them back in a the regex namespace, but ugh.

because then, rollup can optimize using tree-shaking. I haven't done it because the wildcard import was used in other places.

This is a point, but one I'm not 100% sure I'm in a hurry to address, though welcome to discuss it here further since you noticed it and brought it up. :-)

My thoughts:

• regex may eventually get hung off of hljs like everything else... I'm wait to see what "feels right"... but at that point it's just an API of the core library and can't be tree-shaken because we have no idea who might need it.
• Eventually we will build the "common" distributable build (ie, common languages) as one huge module (rather than individual languages as it's built now) and in that case there will be effectively no tree-shaking because the code wouldn't existing in the codebase if it wasn't used SOMEWHERE. And when we build as one huge module the code will also only be included a single time, rather than duplicated.

At that point the only thing tree-shaking would do for us is make the CDN modules a bit "tighter"... and even that wouldn't be true if regex moved back into being hung off of the main library.

Does that make sense?

[nknapp]

Moving it into core makes sense.

I don't if making a huge module makes sense if people use their own bundler. If you just need 3 languages (like in the handlebars website) you probably don't want to have all other rules in the code as well. So it would be better to import distinct rules and let webpack do the, well, tree-shaking. From that point of view it also makes sense not to use the wildcard import.

[nknapp]

Actually I may have spoken too soon because of my last change before 10.0:

  var SHOULD_INHERIT_FROM_HANDLEBARS = hljs.requireLanguage('handlebars');

That would make a hard error if someone wasn't also loading handlebars, although I suppose someone could still be using the file in a manual require pipeline... so perhaps the htmlbars.js file is still required just to cover that edge case.

Is there something to do for me in this respect?

[joshgoebel]

Is there something to do for me in this respect?

Nope. I think the commit I added earlier adding the file back and making it point to handlebars resolves it until v11, when the file can be completely removed.

[joshgoebel]

So it would be better to import distinct rules and let webpack do the, well, tree-shaking.

I’m not sure I follow. Do you think the code should hung off of hljs or be imported? If it hangs off of hljs it cannot be tree-shaken because it’s a public API and the dependencies are not known at compile-time... so all the code has to be included (not that it’s a huge amount of code).

So there are a few different decisions to make here. Ex: If we hang all the util functions off of hljs then how we import them becomes irrelevant.

As as I’ve said elsewhere I’m always hesitant to add more public API because then it’s much easier to add than it is to remove... so I was playing it by ear until some of our 3rd party modules started to want to use this stuff and then see how it works.

I’m more interested in what is clean and simple to do vs shrinking an 100kb library another 3kb or something trivial. Again keeping common use cases in mind,not edge cases.

[joshgoebel]

If a size critical build was really an issue there are much better places to look for savings than whether all the functions in regex.js are tree-shaken or not.

[nknapp]

You are right in that there is not much to gain and I wonder if this discussion is really worth it.

I understand your arguments and I don't want to tell you how to build highlight.js. I just wanted to point out that people may be using ESM-import directly.

Since the core actually does an import * as regex from './lib/regex' the tree-shaking would not change much in this particular case. But there may be other cases in the future where it becomes relevant, who knows.

But I don't want to refactor highlightjs. I think you have done a great job moving to ESM in the first play. And to be honest, I am happy, when this PR is done and out of my mind ✔️

[joshgoebel]

And to be honest, I am happy, when this PR is done and out of my mind ✔️

Sorry it got so complicated and I took so long to come back to it. :-) It's awesome to finally get this finished.

[joshgoebel]

Would you mind writing up a changelog entry (or a few)? I was going to just do it myself but then wasn't sure I could do it all justice... since my big-picture take is that we finally killed htmlbars but I think this makes a lot of other improvements that perhaps you'd like to mention?

[joshgoebel]

@nknapp Take a deep breather and have a cold beverage today. :-) Thanks again!

[nknapp]

I pushed an entry that contains that things that are important to me. But I haven't mentioned htmlbars, because I'm not sure what to write about it exactly.

Should I rebase and squash the commits into semantically relevant ones?

Oh, you merged already 😁

While I was writing this, the label went from "open" to "merged"

[joshgoebel]

But I haven't mentioned htmlbars

I added an entry for the deprecation.

[joshgoebel]

Now we need someone as tenacious as you to tackle all the CSS stuff. :-)