Update "handlebars" to v4.0.6#397
Conversation
|
@Turbo87 - LGTM, can you prepare the corresponding Ember PR also and confirm all works well? |
|
LGTM, I'd like @mmun to review / confirm also. |
|
I think we should address the things listed in #224 before landing this. From @mmun in that issue:
|
| PartialStatement: function(partial) { | ||
| appendChild(this.currentElement(), partial); | ||
| return partial; | ||
| let { name, loc } = partial; |
There was a problem hiding this comment.
No, we will throw for PartialStatement, Decorator, DecoratorBlock. Possibly more, but at least these.
| test("Handlebars partial should error", function(assert) { | ||
| assert.throws(() => { | ||
| parse("{{> foo}}"); | ||
| }, Error(`Handlebars partials are not supported: "{{> foo" on line "1:0"`)); |
There was a problem hiding this comment.
@locks IMHO on line "1:0" looks a bit weird.
- if we say
on linewe should just print the line but not the column - if we said
at "1:0"people might not understand that we are talking about a code location
maybe just append (at 1:0) at the end? or go with the current style and leave out the column?
There was a problem hiding this comment.
Ya, we should say on line X column Z or at L1:C2 (we do the latter in a number of ember's internal errors/warnings).
There was a problem hiding this comment.
I was copying https://github.com/tildeio/glimmer/pull/397/files#diff-96d3a15c8247549ce9be44ee089dc869L132. I'll do at L1:C2 if Ember has that.
| test("Handlebars decorator block should error", function(assert) { | ||
| assert.throws(() => { | ||
| parse("{{#* foo}}{{/foo}}"); | ||
| }, new Error(`Handlebars decorator blocks are not supported: "{{#* foo}}{{/foo}}" at L1:C0`); |
There was a problem hiding this comment.
Not 100% happy with the error message, seems a bit confusing to include the closing tag? I'll tweak. Please comment 😁
There was a problem hiding this comment.
Updated to match partials, let me know if I'm missing anyway, otherwise seems good to go.
5 participants
related to #224
/cc @mmun @rwjblue