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

Provide a way to enable gjs/gts/<template> tag processing without extending plugin:ember/recommended #1895

Open
chancancode opened this issue Jun 20, 2023 · 5 comments

Comments

@chancancode
Copy link

These are the eslint config that is required to make .gjs/.gts work out of the box:

https://github.com/ember-cli/eslint-plugin-ember/blob/master/lib/config/recommended.js#L21-L38

However, these are currently inside the plugin:ember/recommended rules. If a project doesn't extend that config for whatever reason, then it seems like the only option is to find and replicate this in their config. (In this case, if you are curious, I am looking at Discourse, and it seems like they have legacy code that triggers hard parsing errors in some of the rules in recommended.)

Is it possible to move this into the plugin itself or otherwise split it out to plugin:ember/core or something so that users in that camp won't have to find/understand/maintain the setting? (Not super familiar with how things work in eslint to provide a concrete suggestion, sorry!)

@bmish
Copy link
Member

bmish commented Jun 20, 2023

How often does someone use eslint-plugin-ember without extending from our recommended config? I assume the vast majority of users use our recommended config and just disable a few rules they might not like.

However, I will note that we previously had a base config which I removed in #1518 to simplify things. I haven't heard of any use cases for needing a base config, and no complaints since removing it, but your request is a potential use case for it.

Options:

  1. Add back a base config
  2. Just copy this override for a one-off need
  3. Export just this one override so it could be used like this overrides: [require('eslint-plugin-ember'].gjsOverride]

@chancancode
Copy link
Author

I am not sure about how often, I would say that in this case, it seems like discourse is currently unable to use it because they have legacy syntax that causes the rules to error (I haven't investigated much more).

Personally, the idea of a base/minimal config for this kind of stuff seems to make sense.

The issue with copy-and-pasting is that the __GLIMMER_TEMPLATE stuff is essentially an internal implementation detail of the ember eslint plugin that IMO we should try to hide as much as possible. (I know technically it's an implementation detail of the utilities borrowed from ember-template-import, but from the user's perspective that should probably be considered an internal dependency here.) Especially so since that implementation detail is subject to (and probably about to) change.

I think exporting the overrides is okay, but it seems a bit unidiomatic for eslint in my experience, and from what I understand that's essentially what the "extends" stuff is for. It seems like that's basically inventing another way to add back the base config without adding back the base config :)

@bmish
Copy link
Member

bmish commented Jun 20, 2023

It's a fair point that a legacy codebase may want to use a base config and selectively enable some rules on top of that, since our recommended config does target relatively modern versions of Ember (generally LTS versions from the past few years). However, I would still encourage any legacy codebase to flip this around--it's usually best to enable the recommended config and then selectively disable incompatible rules--but I understand not everyone will follow that best practice of mine.

I agree that the solutions of copy-pasting and exposing an individual override are hacky for the reasons you mentioned. Those options could still be useful workarounds if we believed this was a rare, one-off need that we didn't want to officially support nor encourage. If, however, we decide we want to officially support this use case, then yes I agree restoring the base config is the right way to do it.

@bmish
Copy link
Member

bmish commented Jun 20, 2023

If you want to proceed with base, it should mostly just be a revert of #1518.

@chancancode
Copy link
Author

If you think that is a good/appropriate solution, happy to open a PR!

chancancode added a commit to tildeio/eslint-config-discourse that referenced this issue Jun 20, 2023
See the comments and the linked issues for additional context.

I arbitrarily decided to put the template tag at the top of the
class but happy to change that to whatever seems reasonable.

ember-cli/eslint-plugin-ember#1895
ember-cli/eslint-plugin-ember#1896
CvX pushed a commit to discourse/lint-configs that referenced this issue Jun 21, 2023
See the comments and the linked issues for additional context.

I arbitrarily decided to put the template tag at the top of the
class but happy to change that to whatever seems reasonable.

ember-cli/eslint-plugin-ember#1895
ember-cli/eslint-plugin-ember#1896
@bmish bmish mentioned this issue Oct 31, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants