Support <template> (no-undef, etc)#1395
Conversation
template-vars
45d822c to
7e6b83e
Compare
84f716e to
477e98c
Compare
template-vars<template> (no-undef, etc)
477e98c to
246e752
Compare
be53e8a to
4e32eaf
Compare
so close. lol |
|
Green! |
bmish
left a comment
There was a problem hiding this comment.
Can you explain the semver implications of this PR (aside from dropping support for old Node/ESLint versions which we will do separately/regardless)?
And can you talk about the timeline given that the RFC hasn't been approved yet? We don't want to merge support for a feature that hasn't been approved yet, correct?
| "coverageThreshold": { | ||
| "global": { | ||
| "branches": 96, | ||
| "branches": 95, |
There was a problem hiding this comment.
I prefer not to lower this of course.
There was a problem hiding this comment.
I don't know how to get it higher without arbitrarily adding more tests to unrelated-to-this PR :-\
It's now in final comment period! 🎉
The capabilities (I think) are only additive, and no one expecting semver guarantees is currently using gjs/gts, so |
|
Just tried this in a real project over here:
And there are errors -- so it's possible I've misconfigured the tests, or have missed something in my eslint configs, so.. fyi, I guess. I'll report back when I can successfully run eslint with gjs/gts in a real project |
5b4cb7b to
9ec4cb2
Compare
| // checking if results is empty / length === 0 | ||
| let message = ''; | ||
|
|
||
| // When node 12 is dropped, change this to optional chaining |
There was a problem hiding this comment.
Node 12 has been dropped already.
|
Will unused variable detection come into play at all here (this PR or future PR)? There's an ESLint API for helping the no-unused-vars rule detect if variables are used, e.g. in the react/jsx-uses-var rule (implementation). |
|
@bmish we don't need to worry about unused vars in JS because eslint takes care of it (eslint sees the raw format, rather than the |
|
Good news! tested this on a project with gjs -- things work, which is nice. const one = 1;
const two = 2;
<template>
{{one}}
</template>❯ yarn/pnpm/npm lint:js --fix
$ eslint . --cache --fix
<repo>/app/components/test.gjs
2:7 error 'two' is assigned a value but never used no-unused-vars
✖ 1 problem (1 error, 0 warnings)}
Bad news is that prettier doesn't know how to parse gjs, so to lint gjs, I had to remove: from my config. So, I think that means we need to PR parser support to prettier |
|
Here is a more robust solution: adding this entry to the app + addon blueprint's .eslintrc.js config: {
files: ['**/*.gjs', '**/*.gts'],
rules: {
'prettier/prettier': 'off',
}
}, |
4ca5b5c to
713fcfe
Compare
|
Can you merge/rebase the latest master? |
| @@ -1,4 +1,5 @@ | |||
| const rules = require('../recommended-rules'); | |||
| const util = require('ember-template-imports/src/util'); | |||
There was a problem hiding this comment.
(not-blocking)
I see there are several times we have to reach directly into ember-template-imports via file path to import something. The authors of that package may want to consider adding official exports for them so we can import them like this for example:
const { TEMPLATE_TAG_PLACEHOLDER, preprocessEmbeddedTemplates } = require('ember-template-imports');That would go nicely together with strictly-defining their node API via package.json exports which they might also decide to do at some point (many packages have been doing this including ESLint recently).
CC: @chriskrycho
There was a problem hiding this comment.
IMO, it's private API (or at the very least: intimate), so I don't think we actually want to expose it formally.
0498932 to
f370573
Compare
|
Rebased |
Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
f370573 to
85e35d7
Compare
* master: Support `<template>` (no-undef, etc) (ember-cli#1395)
Per RFC: emberjs/rfcs#779
Overall plan, and maybe this is silly, but:
Supports parsing
<template>and discovering if tokens are violating no-undef, etcSomething to keep an eye on in the future: