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

Support <template> (no-undef, etc) #1395

Merged
merged 10 commits into from Oct 27, 2022

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Dec 18, 2021

Per RFC: emberjs/rfcs#779

Overall plan, and maybe this is silly, but:

  • for *gjs and *gts files, use a parser plugin, similar to eslint-plugin-markdown

Supports parsing <template> and discovering if tokens are violating no-undef, etc

Something to keep an eye on in the future:

@bmish bmish changed the title Begin implementing support for template imports Add new rule template-vars Dec 23, 2021
lib/recommended-rules.js Outdated Show resolved Hide resolved
lib/rules/template-vars.js Outdated Show resolved Hide resolved
@NullVoxPopuli
Copy link
Contributor Author

Jest: "global" coverage threshold for branches (96%) not met: 95.98%

so close. lol

@NullVoxPopuli
Copy link
Contributor Author

Green!

Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -47,7 +47,7 @@
"jest": {
"coverageThreshold": {
"global": {
"branches": 96,
"branches": 95,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not to lower this of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to get it higher without arbitrarily adding more tests to unrelated-to-this PR :-\

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Feb 19, 2022

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?

It's now in final comment period! 🎉

Can you explain the semver implications of this PR (aside from dropping support for old Node/ESLint versions which we will do separately/regardless)?

The capabilities (I think) are only additive, and no one expecting semver guarantees is currently using gjs/gts, so
once people start to author gjs/gts, they'll have ESLint run on those files (currently ESLint skips those files).

NullVoxPopuli added a commit to NullVoxPopuli/limber that referenced this pull request Feb 25, 2022
@NullVoxPopuli
Copy link
Contributor Author

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

// checking if results is empty / length === 0
let message = '';

// When node 12 is dropped, change this to optional chaining
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node 12 has been dropped already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call! fixed

@bmish
Copy link
Member

bmish commented Sep 13, 2022

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).

@NullVoxPopuli
Copy link
Contributor Author

@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 <template> stuff) 🎉

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Sep 20, 2022

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:

    'plugin:prettier/recommended',

from my config.

So, I think that means we need to PR parser support to prettier

@NullVoxPopuli
Copy link
Contributor Author

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',
      }
    },

@bmish
Copy link
Member

bmish commented Oct 12, 2022

Can you merge/rebase the latest master?

lib/preprocessors/glimmer.js Outdated Show resolved Hide resolved
@@ -1,4 +1,5 @@
const rules = require('../recommended-rules');
const util = require('ember-template-imports/src/util');
Copy link
Member

@bmish bmish Oct 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it's private API (or at the very least: intimate), so I don't think we actually want to expose it formally.

lib/config/recommended.js Show resolved Hide resolved
lib/config/recommended.js Show resolved Hide resolved
lib/preprocessors/glimmer.js Show resolved Hide resolved
lib/preprocessors/glimmer.js Show resolved Hide resolved
lib/preprocessors/glimmer.js Show resolved Hide resolved
@NullVoxPopuli
Copy link
Contributor Author

Rebased

lib/preprocessors/glimmer.js Outdated Show resolved Hide resolved
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@bmish bmish merged commit 9864569 into ember-cli:master Oct 27, 2022
bmish added a commit to bmish/eslint-plugin-ember that referenced this pull request Oct 27, 2022
* master:
  Support `<template>` (no-undef, etc) (ember-cli#1395)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants