Add mechanism to disable rules inline within a given block. about ember-template-lint HOT 34 CLOSED

OK, I went spelunking to see how hard it would be to fix things to be able to use Handlebars comments. Turns out I am dumb and it was simply not fully implemented in HTMLBars / Glimmer, but is not a hard restriction on the Handlebars parser side. I am working up a PR to Glimmer to enable the following for us (will still need a bunch of work in ember-template-lint after the upstream fixes land).

Current proposal:

<div>
  {{! template-lint disabled=true }}
  {{! Things are disabled here }}
</div>
{{! But not disabled here }}

{{#foo-bar as |baz|}}
  {{! template-lint invalid-interactive=false }}
  {{! invalid-interactive is disabled here }}
{{/foo-bar}}
{{! invalid-interactive is reset to its original / global configuration here }}

<div>
  <div {{! template-lint rule='invalid-interactive' disabled=true}}>
    {{! invalid-interactive *not* disabled }}
  </div>
</div>

<div>
  <div {{! template-lint rule='invalid-interactive' disabled=true recursive=true}}>
    {{! invalid-interactive *is* disabled }}
  </div>
</div>

This proposal has the following additional benefits:

• We can deprecate <!-- template-lint, and rely only on Handlebars comments (this has irked me for quite some time).
• We do not have to worry about changing semantics (i.e. changing the comments to be block scoped by default)

from ember-template-lint.

@bendemboski - Love it! Note: the element modifier approach will only work for actual DOM elements, not component invocations (mostly because I cannot even reason about what that would mean). I also think I would definitely still want the original proposed syntax also.

Current proposal:

{{#template-lint disabled=true}}
  {{! Things are disabled here }}
{{/template-lint}}

{{! But not disabled here }}

<div>
  <div {{template-lint rule='invalid-interactive' disabled=true}}>
    {{! invalid-interactive *not* disabled }}
  </div>
</div>

<div>
  <div {{template-lint rule='invalid-interactive' disabled=true recursive=true}}>
    {{! invalid-interactive *is* disabled }}
  </div>
</div>

Everyone on board with that?

from ember-template-lint.

@mixonic / @mmun - Thoughts?

from ember-template-lint.

I like it.

from ember-template-lint.

I like this a lot too. However, I have a different use case in mind that is maybe related that I'd like to mention. If it better belongs in a separate issue, I'll happily take it there, but it's related so I think at least worth considering at the same time as this initially.

The current mechanism allows users to disable rules for a range of lines in a file. Embracing the structure of HTML and allowing a disabling mechanism that operates on DOM nodes/trees rather than lines of text is a fantastic idea. This proposal provides a mechanism for disabling a rule for a given sub-tree of the DOM, but often rather than that I just want to disable it for a given DOM element (and not its descendants).

This is especially relevant when the rule addresses element attributes. For example, I recently had a discussion about invalid-interactive where sometimes I want to disable it for a specific element, but make no assumptions about if/how the rule should apply to the elements it contains.

In order to make this syntax support this, I think we'd need to support something like this (a very small variant of the examples shown in this issue and the jsbin):

{{#template-lint rule='invalid-interactive' disabled=true}}
  <div onscroll={{action 'onScroll'}}>
    {{#template-lint rule='invalid-interactive' disabled=false}}
      {{! ... }}
    {{/template-lint}}
  </div>
{{/template-lint}}

This would work, but seems overly verbose for what I think might be a fairly common use case of disabling a rule for an element rather than a DOM sub-tree. What do folks think about supporting some other syntax for disabling rules for a single element? Maybe the syntax could be something like:

<div>
  <div {{template-lint rule='invalid-interactive' disabled=true}}>
    {{! invalid-interactive *not* disabled }}
  </div>
</div>

Or perhaps there's a better syntax? I suppose if we're doing that we could also support a recursive (or subtree or something) flag in addition to or instead of the original proposal in this issue:

<div>
  <div {{template-lint rule='invalid-interactive' disabled=true recursive=true}}>
    {{! invalid-interactive *is* disabled }}
  </div>
</div>

Thoughts?

from ember-template-lint.

@bendemboski Your proposal is very similar to the element modifiers RFC. I quite like your final iteration but I believe that all three invocation patterns should possibly be supported.

from ember-template-lint.

Oh yeah, thinking about it, we'd definitely still want to support the originally proposed syntax because you might not always want to disable for a single DOM sub-tree, but maybe:

<div>
  <h1>{{heading}}</h1>
  {{#template-lint rule='bare-strings' disabled=true}}
    <p>Content!</p>
    <p>More content!</p>
  {{/template-lint}}
  <button>{{actionText}}</button>
</div>

or even

<div>
  {{someText}}
  {{#template-lint rule='bare-strings' disabled=true}}
    and that's all I have to say about that.
  {{/template-lint}}
</div>

from ember-template-lint.

lol:

{{#template-lint rule='bare-strings' disabled=true}}
  and that's all I have to say about that.
{{/template-lint}}

I'm quite pleased with the current recommendation from @bendemboski and thank you for succinctly summarizing it, @rwjblue. 👍

from ember-template-lint.

I'd prefer to see as much of this as possible done in comments, HTML or handlebars. This is partially b/c comments already communicate that the meta-data will be stripped from final output.

For example if someone not super familiar with this system sees: {{#template-lint}}foo{{/template-lint}} Ember has taught them elsewhere they should expect output of <div>foo</div>. However that isn't actually doing to happen.

Furthermore, linter config in JavaScript (eslint, jshint) has always been based on comments. This seems worth mirroring just to be consistent.

I think an alternative could be arrived at using either HTML comments or HBS comments.

from ember-template-lint.

An example using HBS comments:

<div>
  <div {{! template-lint rule='invalid-interactive' disabled=true}}>
    {{! invalid-interactive *not* disabled }}
  </div>
</div>

Or using HTML:

<div>
  <!-- template-lint-element rule='invalid-interactive' disabled=true -->
  <div>
    {{! invalid-interactive *not* disabled }}
  </div>
</div>

The block case is a bit more tricky.

from ember-template-lint.

Anybody care to specify block comments in Handlebars as part of this effort? 😜 It seems like that could be:

{{!#}}
  Anything
  {{here}}
  <div>disappears.</div>
{{!/}}

And then you can name them if you want to allow for nested directives which don't end at the nearest block (adopts requirement for matching beginning and ending):

{{!#name attribute=foo}}
  {{!#name attribute=bar}}this thing doesn't close the outer comment block =>{{!/name}}
{{!/name}}

from ember-template-lint.

@mixonic:

We cannot use HBS comments, they are stripped by Handlebars parser and not available at the AST plugin stages. HTML comments would be fine I suppose, but because we cannot get a block form I would want to scope them to the "current block" level.

Using HTML comments is (and has been) annoying, because we have an explicit rule disallowing them for everything OTHER THAN template linting control comments. This has been ok so far, so maybe its fine to keep going that way...

Follow up proposal using HTML comments that are stripped:

<div>
  <!-- template-lint disabled=true -->
  {{! Things are disabled here }}
</div>
{{! But not disabled here }}

{{#foo-bar as |baz|}}
    <!-- template-lint invalid-interactive=false -->
   {{! invalid-interactive is disabled here }}
{{/foo-bar}}
{{! invalid-interactive is reset to its original / global configuration here }}

Note: this does not allow @bendemboski's suggestion, because HTML comments are not allowed inside an element:

  <div <!-- foo --> data-bar="baz">
  </div>

from ember-template-lint.

We cannot use HBS comments, they are stripped by Handlebars parser and not available at the AST plugin stages.

That's right, I knew I must have been forgetting something 😢

from ember-template-lint.

We cannot use HBS comments, they are stripped by Handlebars parser and not available at the AST plugin stages.

That's right, I knew I must have been forgetting something

FWIW, I am pestering @mmun to see if he has any insight in if this is possible to change/fix, but we should not assume it is possible just yet...

from ember-template-lint.

If we're unwilling to use element modifiers then I have one other thought for how to support my use case. Something like:

<!-- template-lint invalid-interactive=false scope="next-element" -->
<div data-explanation="invalid interactive is disabled here">
  <div data-explanation="invalid interactive is *not* disabled here"></div>
  <div data-explanation="invalid interactive is *not* disabled here"></div>
</div>
<div data-explanation="invalid interactive is *not* disabled here"></div>

and then if we want:

<!-- template-lint invalid-interactive=false scope="next-element-tree" -->
<div data-explanation="invalid interactive is disabled here">
  <div data-explanation="invalid interactive is disabled here"></div>
  <div data-explanation="invalid interactive is disabled here"></div>
</div>
<div data-explanation="invalid interactive is *not* disabled here"></div>

I don't like it as much as using element modifiers but @mixonic has some good points.

from ember-template-lint.

Only limitation, the latest proposal doesn't allow for:

{{!#template-lint invalid-interactive=false}}
{{#foo-bar as |baz|}}
  {{! invalid-interactive is disabled here }}
{{/foo-bar}}
{{#foo-bar as |baz|}}
  {{! invalid-interactive is disabled here }}
{{/foo-bar}}
{{!/template-lint}}

Want to make sure we're explicitly opting into that.

from ember-template-lint.

I think it also no longer supports a case that is currently supported:

<!-- template-lint invalid-interactive=false -->
<div>
  {{! invalid-interactive *is* disabled }}
  <div>
    {{! invalid-interactive *is* disabled }}
    <div>
      {{! invalid-interactive *is* disabled }}
      <!-- template-lint invalid-interactive=true -->
      {{! invalid-interactive *not* disabled }}
    </div>
    {{! invalid-interactive *not* disabled }}
  </div>
  {{! invalid-interactive *not* disabled }}
</div>

I am 1000% on board with deprecating that because I think we should be operating on DOM nodes/trees not lines of text, but just wanted to call it out to be explicit.

from ember-template-lint.

FYI - Submitted glimmerjs/glimmer-vm#340 as a WIP Glimmer PR to add the support we need.

@nathanhammond - RE: https://github.com/rwjblue/ember-template-lint/issues/79#issuecomment-258972873, we could make the following work properly:

{{! template-lint invalid-interactive=false}}
{{#foo-bar as |baz|}}
  {{! invalid-interactive is disabled here }}
{{/foo-bar}}
{{#foo-bar as |baz|}}
  {{! invalid-interactive is disabled here }}
{{/foo-bar}}
{{! template-lint invalid-interactive=true }} 

@bendemboski - RE: https://github.com/rwjblue/ember-template-lint/issues/79#issuecomment-258974260, agreed but that confuses the heck out of me and I think not supporting it is a feature 😝 . The following would be supported though:

{{! template-lint invalid-interactive=false }}
<div>
  {{! invalid-interactive *is* disabled }}
  <div>
    {{! invalid-interactive *is* disabled }}
    <div>
      {{! invalid-interactive *is* disabled }}
      {{! template-lint invalid-interactive=true }}
      {{! invalid-interactive *not* disabled }}
    </div>
    {{! template-lint invalid-interactive=true }}
    {{! invalid-interactive *not* disabled }}
  </div>
  {{! template-lint invalid-interactive=true }}
  {{! invalid-interactive *not* disabled }}
</div>

Its more verbose, but I feel it is more obvious. Also, the only reason the original snippet in your comment worked was because we do a depth first traversal. If we instead changed the traversal to be breadth first, that would break.

from ember-template-lint.

@rwjblue I'm assuming that you mean for your example in your last post to follow the convention of "must be at the same level" which you imply in the second half of your comment to Ben?

{{! template-lint invalid-interactive=false}}
{{#foo-bar as |baz|}}
  {{! invalid-interactive is disabled here }}
{{/foo-bar}}
{{#foo-bar as |baz|}}
  {{! invalid-interactive is disabled here }}
{{/foo-bar}}
{{! template-lint invalid-interactive=true }} 

from ember-template-lint.

@nathanhammond - Yes, exactly.

from ember-template-lint.

@rwjblue I entirely agree, just wanted to be clear that if anybody is using the structure in https://github.com/rwjblue/ember-template-lint/issues/79#issuecomment-258974260 they'll have to make a not-quite-mechanical change to update it.

So, the principle behind this (i.e. how we think of it applying to the DOM tree) is that we're providing two ways of configuring the linter in a template:

1. Add an instruction as an element attribute that applies to it and (optionally) to its descendants.
2. Add an instruction as a child-less node that applies to all of its later siblings nodes and (mandatorily) to their descendants.

And we're abandoning @rwjblue's original thought about adding an instruction as a node that can have children and applies only to its descendants ({{#template-lint}}...{{/template-lint}}) so that we can more easily implement this using handlebars comments for clarity. Or maybe someday we'll do https://github.com/rwjblue/ember-template-lint/issues/79#issuecomment-258942645 and hope that we haven't made the linter so smart that a cyborg from the future comes back to destroy it.

Sound right? It's gotten a little more intimidating, but I'm still willing to take it on unless anybody thinks I shouldn't.

from ember-template-lint.

Sound right?

Yes, I believe this is a good summary of my current line of thinking. I've updated the main description above to reference the current state of the proposal.

It's gotten a little more intimidating, but I'm still willing to take it on unless anybody thinks I shouldn't.

Nah, it'll be easy. We can work on some mockup JSBin's as soon as glimmerjs/glimmer-vm#340 lands.

from ember-template-lint.

In the meantime we should decide what the interface for the inline HBS comments are. I suspect that we want the ability to do more than just set rules to true and false (or disable all), so we will need to decide what the serialization format is. The easiest thing to do is just JSON.parse, but I'm not sure that feels natural...

The strawman (we would use JSON.parse to get the values back out):

{{! template-lint "{ disable: true, 'foo-bar'='derp' }" }}

Note: I think ^^ looks like garbage 😈

from ember-template-lint.

https://github.com/rwjblue/ember-template-lint/pull/130 should make things a bit easier when glimmerjs/glimmer-vm#340 lands.

from ember-template-lint.

I don't really like spreading state around, but one option would be to allow inline configuration of simple key/value pairs, and then support more complex configurations with named configurations in the global config file that can be applied from comments.

// .template-lintrc.js
module.exports = {
  rules: {
    "nested-interactive": true,
    "invalid-interactive": true,
    "bare-strings": true
  },
  localConfigs: {
    "interactive-divs": {
      "nested-interactive": {
        ignoredTags: ["div"]
      },
      "invalid-interactive": {
        ignoredTags: ["div"]
      }
    },
    "i-do-what-i-want": {
      "nested-interactive": false,
      "invalid-interactive": false,
      "bare-strings": false
    }
  }
};

{{! template.hbs }}
<div>
  {{! template-lint "interactive-divs" }}
  <div onscroll={{action 'onScroll'}}></div>
  <a href="#"><button>{{actionText}}</button></a>
  <div>
    {{! template-lint bare-strings=false triple-curlies=false }}
    Look, a bare string!
    {{{escapedHtml}}}
  </div>
</div>
<div>
  {{! template-lint "i-do-what-i-want" }}
  <div onclick={{action 'onClick'}}>Click me even though I'm a div!</div>
  <a href="#"><button>What up, I'm nested!</button></a>
  Bare text here, what are you gonna do about it ember-template-lint? That's right, nothing!
</div>

from ember-template-lint.

Actually I think something like that will be required if we ever want to support rules who are configured with non-serializable values such as functions, which seems reasonable. In that case we either have to move the configuration out of the template into a Javascript file, or else work out some syntax where we parse the comments and throw the results at eval, which makes me feel dirty to even contemplate.

from ember-template-lint.

It seems like named states are a good escape hatch and inline config can either be foo=bar or a simple JSON hash. I don't have a ton of preference here...

from ember-template-lint.

@bendemboski - To aid in working on this, I made a branch that you can target that has the required upstream changes (by publishing that PR as rwjblue-glammer-engine):

https://github.com/rwjblue/ember-template-lint/tree/handlebars-comments

from ember-template-lint.

@rwjblue thanks! I still don't quite get the significance of that change, though. Does it give me new/different AST APIs to use or something? Or is it just a code organization change?

from ember-template-lint.

@bendemboski - There will be a new MustacheCommentStatement visitor that runs for the Handlebars comments (there is already a CommentStatement visitor you can use, but that is only for HTML comments).

from ember-template-lint.

Okay, so then I propose three different instruction syntaxes (inspired by eslint):

Enable/disable all rules:

{{! template-lint-disable }}
{{! template-lint-enable }}

Enable/disable one or more rules:

{{! template-lint-disable bare-strings }}
{{! template-lint-enable triple-curlies nested-interactive }}

Configure one rule at a time:

{{! template-lint-configure bare-strings ["?", "1"] }}
{{! template-lint-configure nested-interactive {"ignoreTabIndex": true} }}
{{! template-lint-configure complex-rule {"key": {"nested": true} } }}

Note that care has to be taken in the last one to not have the rule JSON accidentally end the handlebars comment prematurely. Fortunately, if that were to happen, the JSON would not parse so it would definitely not fail silently.

Also, because JSON the last one would support an alternate rule toggling syntax for free:

{{! template-lint-configure bare-strings false }}

So we could do without the second, but I think it's a nice syntax to have anyway.

Sound good? @rwjblue @nathanhammond

from ember-template-lint.

Sounds perfect.

from ember-template-lint.

Hrm, I just realized, that's problematic for supporting the 'recursive' flag on in-element instructions. How about we do away with that, and to support that case you'd just have to do the slightly more verbose

<div>
  <div></div>
  <div {{! template-lint-disable invalid-interactive }}>
    {{! template-lint-disable invalid-interactive }}
    <div></div>
  </div>
  <div></div>
</div>

or

<div>
  <div></div>
  {{! template-lint-disable invalid-interactive }}
  <div>
    <div></div>
  </div>
  {{! template-lint-enable invalid-interactive }}
  <div></div>
</div>

Unless somebody has an idea for a clean semantic for parsing options meant for "rule scoper" as separate from the JSON meant for the rule itself.

from ember-template-lint.

@bendemboski - I didn't think that we were talking about template-lint-enable / template-lint-disable parsing JSON? If that is true (that they will not be JSON, just a space separated list of rule names) then the more verbose version will only be needed when you need to do {{! template-lint-configure bare-strings ['.'] }}.

We should also queue up a deprecation for the <!-- template-lint style comments (since we won't need them any more).

Also, the changes needed upstream to support Handlebars comments landed in [email protected] and that was merged here in https://github.com/rwjblue/ember-template-lint/pull/133, so you should be good to go...

from ember-template-lint.