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

(htmlbars) should inherit from Handlebars #2181

Closed
nknapp opened this issue Oct 12, 2019 · 47 comments · Fixed by #2344
Closed

(htmlbars) should inherit from Handlebars #2181

nknapp opened this issue Oct 12, 2019 · 47 comments · Fixed by #2344
Labels
enhancement An enhancement or new feature language
Milestone

Comments

@nknapp
Copy link
Contributor

nknapp commented Oct 12, 2019

I am currently working on refining the handlebars-language to support literal-segments:

The Handlebars-tests are working fine with my changes, but HTMLBars detection testcase no fails... because highlight.js detects Handlebars instead of HTMLBars.

Now, the file that should be detected as HTMLBars, might was well be a Handlebars file

<div class="entry">
  {{!-- only output this author names if an author exists --}}
  {{#if author}}
    {{#playwright-wrapper playwright=author action=(mut author) as |playwright|}}
    <h1>{{playwright.firstName}} {{playwright.lastName}}</h1>
    {{/playwright-wrapper}}
  {{/if}}
  {{yield}}
</div>

In my oppinion, there is no need for an HTMLBars language, because the syntax is actually the same as Handlebars. It even uses the Handlebars parser to generate an AST. Couldn't htmlbars just be an alias of Handlebars.

I think the only difference really lies in the list of built-ins. HTMLBars/Glimmer/Ember seems to have many more builtin-helpers. But it should be possible to somehow handle that.

There are parts of the html-bars definition that should be moved into the handlebars-definition anyway (like sub-expressions).

I am willing to put some work into that. I just wanted to ask for your opinions first.

@joshgoebel
Copy link
Member

joshgoebel commented Oct 12, 2019

I was thinking about this the other day actually. I think there are two ways to go here:

1. Merge them and make the master a superSet... so "htmlbars" would become an alias.

I think this sounds good at first glance, but then I think it starts to get complex later... like how we're forcing Javascript to be JS, JSX, etc... (the problem there is that JSX is quite a bit more complex than JS, but that might not really be the case with HTMLBars if it's just additional helpers rather than syntax changes) in another world you could have imagined just building JSON into JS if you followed this line of thinking too far.

2. Go with a dependency/clone instead, laying in the new keywords on TOP

I'd like to suggest (or float) an alternative... that HTMLbars be a superset that derives from handlebars... ie it should be a very minimal syntax that deep clones the handlebars syntax and then perhaps adds additional matchers/keywords on top.

This is also how I feel we should be doing cpp and arduino though it's not presently. One benefit is that the more complex grammar just wins by default... if cpp and arduino are the same but arduino scores 5 points higher, very likely it's Arduino code - and even if it's not, not much harm done.

Same in this case I'd think... if those additional built-ins raise the relevance and now we choose the HTMLBars grammar by mistake, not a LOT of harm done... I also think this likely has the advantage of being easier to maintain over time (cleaner separation of concerns)... though it's possible we currently don't have the tools to make this pattern super easy in PRACTICE (but we may).

I wouldn't want Arduino keywords to just merge into cpp, so that's why I tend to lean a little more towards choice 2 here.

@joshgoebel joshgoebel changed the title Merge HTMLBars and Handlebars Discuss: Merge HTMLBars and Handlebars Oct 12, 2019
@joshgoebel joshgoebel added the big picture Policy or high level discussion label Oct 12, 2019
@joshgoebel
Copy link
Member

Read this and see if it changes your mind on anything. Looks like the big change is now you can put the {{}} tags anywhere.

https://colintoh.com/blog/htmlbars

Does that break how subLanguages currently work?

@nknapp
Copy link
Contributor Author

nknapp commented Oct 12, 2019

Reading your comment, the following came to my mind: "htmlbars" actually depends on then parser of Handlebars 3, not the latest version. And there have been changes in Handlebars as well, and I'm not sure if "htmlbars" implements them

Partial blocks

{{#> layout}}
{{> @partial-block }}
{{/layout}}

Inline partials and decorators

{{#*inline "content"}}bar{{/inline}}
{{#*customDecorator}}template{{/customDecorator}}

I actually want to deprecate customDecoratory, but the syntax will remain with inline partials.

Now, this sounds like a base language with two derivates.

I also don't know if I really have the time and energy to implement all those missing things.

@joshgoebel
Copy link
Member

Well, we do have precedent (recent) for "superset" if it "causes no harm"... so we added comments to JSON... since if you have JSON without comments, no harm done - they just aren't parsed/highlighted...

Now, this sounds like a base language with two derivates.

So I'm not sure by saying that if you're agreeing or disagreeing. :-)

@nknapp
Copy link
Contributor Author

nknapp commented Oct 12, 2019

The examples on https://colintoh.com/blog/htmlbars do look like there is no different highlighting needed.

I agree with your second point, that "htmlbars" should extend "handlebars" and define custom keywords. If you think that the additional syntax causes not harm for "htmlbars"-highlighting we can do it like that.

The changes described at https://colintoh.com/blog/htmlbars do not really change anything in terms of highlighting, so it would mainly be the helpers.

@joshgoebel
Copy link
Member

joshgoebel commented Oct 12, 2019

Does the sublanguage stuff already allow this:

<img src={{photo.thumbUrl}}>

I'm not that familiar with sublanguages... my head tells me we'd be in "html' mode and that would get ignored.

@joshgoebel
Copy link
Member

I think this is a little hard now (from looking at ARduino and CPP) but would be much easier if we switched to a modular build system. What I really want (for Arduino) isn't the COOKED CPP data, I want the raw data...

IE, really Arduino needs to require the raw CPP metadata and then lay it's pieces on top... that also avoids any need to deep clone...

Then with only a few lines of code Arduino would require CPP, lay a few keywords on-top, then compile as it's very own language.

Although now I"m wondering if sublanguage might be relevant here. :-)

@nknapp
Copy link
Contributor Author

nknapp commented Oct 12, 2019

<img src="{{photo.thumbUrl}}">

with the handlebars-renderer becomes

<span class="xml"><span class="hljs-tag">&lt;<span class="hljs-name">img</span> <span class="hljs-attr">src</span>=<span class="hljs-string">"</span></span></span><span class="hljs-template-variable">{{photo.thumbUrl}}</span><span class="xml"><span class="hljs-tag"><span class="hljs-string">"</span>&gt;</span>
</span>

and

<img src={{photo.thumbUrl}}>

becomes

<span class="xml"><span class="hljs-tag">&lt;<span class="hljs-name">img</span> <span class="hljs-attr">src</span>=</span></span><span class="hljs-template-variable">{{photo.thumbUrl}}</span><span class="xml"><span class="hljs-tag">&gt;</span>
</span>

In both cases, it looks like the mustaches are wrapped correctly in a <span class="hljs-template-variable.

@joshgoebel
Copy link
Member

Well, I guess I need to go read the parser and pay attention to how subLanguages work. :)

@joshgoebel
Copy link
Member

We'll see if Egor has any concerns but this is what I had in mind for CPP/Arduino:

#2185

Best of both worlds... and now Arduino is really more of a "subclass" of CPP. If we started doing this a lot we could add a little more tooling to make it easier.

@joshgoebel
Copy link
Member

And he had no issues with my Arduino/CPP changes... so if the new Handlebars stuff really covers all the bases now I'd use the same strategy for HTMLBars... just make it a super-set of Handlebars, and then lay the new keywords on-top.

If there are no existing markup tests cases I'd add one or two before the changeover, just to get a working baseline.

@nknapp
Copy link
Contributor Author

nknapp commented Oct 14, 2019

It's a breaking change though, because people who only load the htmlbars language (i.e from a cdn) will have to load handlebars as well then.

@joshgoebel
Copy link
Member

Hmmm. We could just fix that on the cdn by bundling the files though. Needs further thought.

@joshgoebel
Copy link
Member

joshgoebel commented Oct 14, 2019

It's a breaking change though, because people who only load the htmlbars language (i.e from a cdn) will have to load handlebars as well then.

Merging doesn't help either then does it? Because someone might be including whichever file we decide is the "loser" and then it's also a breaking change, yes? Although you could just do the aliasing on the CDN by having a copy of the file I suppose.

@joshgoebel
Copy link
Member

We could also provide warning (release notes, console.log) with a prior release and then in the next release simply do the breaking change... and people would simply have to update.

Of course in the "default" distribution builds we'd just include the proper files. (ie, if it included one, it would include the proper dependencies).

@joshgoebel
Copy link
Member

It's a breaking change though, because people who only load the htmlbars language (i.e from a cdn) will have to load handlebars as well then.

Context: Making HTMLBars depend on handlebars. (which would mean now people using it via CDN would need to include BOTH files)

@egor-rogov Any thoughts on when (if ever) we can break stuff like this? I think every once in a while it should be ok to tell people "hey you might need to pay attention to a few changes"...

Should we queue up such things for a MAJOR release or is in ok to do this in MINOR releases too?

@egor-rogov
Copy link
Collaborator

@egor-rogov Any thoughts on when (if ever) we can break stuff like this? I think every once in a while it should be ok to tell people "hey you might need to pay attention to a few changes"...

I think so either.

Should we queue up such things for a MAJOR release or is in ok to do this in MINOR releases too?

AFAIK we have no releases numbering police. I don't know why we use 3 numbers and when it's time to increment the second, let alone the first. Maybe @marcoscaceres can say more.
Until such policy exists, it's ok for me to do it at any moment provided it is mentioned in CHANGES.

@marcoscaceres
Copy link
Contributor

We tend to follow semantic versioning: https://semver.org/

Last number is bug fix(es), next is new feature(s), and left-most is “BREAKING CHANGE”.

@joshgoebel
Copy link
Member

Well, it's semantic versioning, but you're right in that the definitions of MAJOR MINOR and POINT could be a bit fuzzy... honestly our any day now x.x.11 release should probably be minor IMHO, but I'm just going by my "gut" rather than any big reason why.

So far our features/codebase has been really stable, so if that was the criteria we might be doing point releases for a long time. :-)

I'd actually love something like 2019.1 (first release, 2019) style versioning but I was going to save that suggestion for another day. :-)


But to me it sounds like you'd be more than happy doing such things in minor releases... (I'd personally keep point releases very boring, if we stuck with semantic versioning)...

@joshgoebel
Copy link
Member

joshgoebel commented Oct 26, 2019

@marcoscaceres Is moving/renaming a secondary language file on the CDN a breaking change in your mind?

@marcoscaceres
Copy link
Contributor

marcoscaceres commented Oct 26, 2019

To be clear, it’s never ok to break on features or minor releases because automated systems (including npm) assume things won’t break. If you want to get people’s attention, you need to announce a “BREAKING CHANCE” with a bump to the left most value.

@marcoscaceres
Copy link
Contributor

Is moving/renaming a secondary language file on the CDN a breaking change in your mind?

Will it unexpectedly break any software applications downstream? If yes, then yes :)

@joshgoebel
Copy link
Member

I can live with that, just means we save up the breaking stuff for 10.0...

@egor-rogov
Copy link
Collaborator

We tend to follow semantic versioning: https://semver.org/
Last number is bug fix(es), next is new feature(s), and left-most is “BREAKING CHANGE”.

Is it really so? Why then the current version is 9.15.10? There were plenty of new features since 9.15.0.
Let's switch to 9.16 then.

@marcoscaceres
Copy link
Contributor

#1554 definitely counts as a new feature in that it provides some new functionality. Looking at the change log, the rest appear to be bug fixes (prefixed with “fix(“) and little enhancements.

@joshgoebel
Copy link
Member

Well depends if you call some of them enhancements or fixes... but I don' think we have new "features". I'm hesitant to call the build system or maintenance system real "features". But if everyone likes 9.16 we can do 9.16 lol...

@joshgoebel
Copy link
Member

I have big plans for 1554. Lol.

@egor-rogov
Copy link
Collaborator

#1554 definitely counts as a new feature in that it provides some new functionality. Looking at the change log, the rest appear to be bug fixes (prefixed with “fix(“) and little enhancements.

It's very subjective to my taste. For example 9.14.0 could well be 9.13.2. Not that it matters much (:
My point is: no reason to delay sane changes until the next major release. "Next major" may as well be simply the next. If it breaks something, okay, let's bump the first digit if it helps someone to notice changes.

@nknapp
Copy link
Contributor Author

nknapp commented Oct 27, 2019

#1554 could also be a new project...

@nknapp
Copy link
Contributor Author

nknapp commented Oct 27, 2019

The question is: what is the documented API. Someone should be able to go back from 9.15.11 to 9.15.10 without fearing a breakage. But not from 9.16.0 to 9.15.10

That's the point of the distinction between patch and feature. If someone uses the new cli-tool in a build and goes back to 9.15.10, that would not work.

In case of the language improvements for example, it would only reintroduce old deficits.

Generally, I that think it is save to increase the minor version if there are no breaking changes. Most people use ^ semver patterns anyway

@joshgoebel
Copy link
Member

I think for now anytime I have any "enh()" (enhancement) commits in the changelog I'm going to bump the middle number... but if a release consists ENTIRELY of bug fixes then I'd only bump the minor number.

We can figure out what constitutes a major bump in the near future I think.

@nknapp
Copy link
Contributor Author

nknapp commented Oct 31, 2019

I don't know if you want to do this and I haven't used it myself, but semantic-release automates this decision process...

@joshgoebel
Copy link
Member

joshgoebel commented Oct 31, 2019

Yeah, probably not looking for an automated solution until I feel a little more pain with the current process...Automating the change log from git commits or something MIGHT be nice, but that would require a very specific way of writing the commit history to do it the way I currently do it (which is very specific and subjective).

Perhaps a list of PRs, but even then sometimes I split a PR into multiple notes in the release... etc... just so many other things to focus on. :-)

Incrementing the versions numbers in a few places isn’t the hard part - and that’s really all the current “release” process is that I have to deal with, at least.

@nknapp
Copy link
Contributor Author

nknapp commented Nov 1, 2019

Generally, I that think it is save to increase the minor version if there are no breaking changes. Most people use ^ semver patterns anyway

In order to prevent misunderstandings. What I meant with minor was the middle version.

9 . 15 . 10
^   ^    ^
|   |    |
|   |    \-- patch
|   |
|   \--- minor
|
\--- major

@joshgoebel
Copy link
Member

@nknapp

#2273

What's your recommendation for this when we're ready to finally break things? Should this just become a single grammar with alias? If so which language "wins" to become the actual grammar?

@nknapp
Copy link
Contributor Author

nknapp commented Nov 17, 2019

Not sure... I think the "handlebars"-definition is more sophisticated right now, but there is one rule about subexpressions in the "htmlbars"-definition that might be worth porting. And the "as"-keyword as well.

What about the solution to have a configurable base language and two inheriting languages with a different set of helpers?

@joshgoebel
Copy link
Member

What about the solution to have a configurable base language and two inheriting languages with a different set of helpers?

Not opposed, but I think for something like this we'd use ES6 modules (post new build system), not the runtime requires system (like we do for Arduino/CPP). That stuff still needs to be sorted out.

I don't think users/packagers should have to think that granually though... like "if i want handlebars I need to also load handlerbar-like-base..."...

I was a fan of the consolidate and alias approach but perhaps that's not best. Right now you're the expert with handlebars. :-)

@joshgoebel
Copy link
Member

Not sure... I think the "handlebars"-definition is more sophisticated right now, but there is one rule about subexpressions in the "htmlbars"-definition that might be worth porting. And the "as"-keyword as well.

If you look you'll see SUB_EXPR currently isn't even referenced in the file actually. It's just defined and then not used. It looks like HTMLbars has quite a few additional keywords:

// htmlbars
var BUILT_INS = 'action collection component concat debugger each each-in else get hash if input link-to loc log mut outlet partial query-params render textarea unbound unless with yield view';
// handlebars
var BUILT_INS = {'builtin-name': 'each in with if else unless bindattr action collection debugger log outlet template unbound view yield lookup'};

We could still make htmlbars require handlebars and add onto it if you think that's best?

If you went that route it'd be very similar to what we do now with C++ and Arduino.

@joshgoebel
Copy link
Member

Oh, I don't think you're handling assignments at all?

@nknapp
Copy link
Contributor Author

nknapp commented Dec 7, 2019

Assignments?

@joshgoebel
Copy link
Member

Htmlbars has attribute assignments as it’s own thing I think?

@joshgoebel
Copy link
Member

@nknapp You interesting in more work on this? I dug a little and to me it looks like technically the syntax of handlebars supports almost all the stuff that HTMLbars already highlights, so currently the handlebars grammar is actually the one lacking.

Handlebars does a better job of dealing with the many types of tags and proper escaping, but HTMLbars does a better job with the inner content (strings, numbers, attribute names)... it should be pretty easy to move all that support into handlebars also then just make HTMLbars a direct dependency that adds a few keywords.

If you brought the base handlebars syntax up to speed I could do the dependency work.

@joshgoebel
Copy link
Member

If you don't have time and egor is game we might just "tentatively" attach these for v10 even if the work isn't done to get that breaking change out of the way so the work can be done later. Right now from where I set it absolutely needs this should really be one language definition not two.

@nknapp
Copy link
Contributor Author

nknapp commented Dec 25, 2019

Do you have a timeline/plan, when this should be done? I would be able to do some work here... After Christmas...

@joshgoebel
Copy link
Member

Well v10 will land sometime in Jan... I think I put Jan 18 on the Milestone right now.

@joshgoebel joshgoebel added language and removed big picture Policy or high level discussion labels Dec 26, 2019
@joshgoebel joshgoebel changed the title Discuss: Merge HTMLBars and Handlebars HTMLBars inherits from Handlebars Dec 26, 2019
@joshgoebel joshgoebel changed the title HTMLBars inherits from Handlebars (htmlbars) should inherit from Handlebars Dec 26, 2019
@joshgoebel joshgoebel added the enhancement An enhancement or new feature label Dec 26, 2019
@nknapp
Copy link
Contributor Author

nknapp commented Jan 2, 2020

I will start working on this now. I don't know how much time I can spend and if Jan 18 is a feasible deadline. But I can at least start.

@joshgoebel
Copy link
Member

I don't think it's actually that much work, but it's also easy to say that until you got bogged down in the weeds. :) Good luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants