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

Add new rule no-at-ember-render-modifiers #1902

Merged
merged 10 commits into from Jul 7, 2023

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Jun 26, 2023

Closes: #1901

@bmish bmish added Enhancement New Rule Idea for a new lint rule labels Jun 26, 2023
@lin-ll
Copy link
Collaborator

lin-ll commented Jun 30, 2023

I support this rule - I'm just curious about how we've historically handled rules like this which could also just be configured via no-restricted-imports or something. @bmish any thoughts? I wonder if it would be beneficial to export a "suggested" no-restricted-imports configuration from this plugin

@bmish
Copy link
Member

bmish commented Jun 30, 2023

I support this rule - I'm just curious about how we've historically handled rules like this which could also just be configured via no-restricted-imports or something. @bmish any thoughts? I wonder if it would be beneficial to export a "suggested" no-restricted-imports configuration from this plugin

A suggested no-restricted-imports configuration is an interesting idea, although I'd be worried this would go unnoticed by most users and the configuration could get unwieldy, but it's worth considering. I generally consider no-restricted-imports to only suffice for one-off customizing allowed imports in one's own codebase.

When we want to issue a community-level edict disallowing a particular dependency, I think at that point we may deserve to have an actual rule that we can document, test, that can be part of the recommended config, and that users can easily turn on or off without copy-pasting config into no-restricted-imports.

However, the problem with this that I haven't reconciled yet is that there are likely dozens of Ember-related dependencies that are deprecated at this point, and we probably don't want to pollute our list of rules with a single rule for each one. Perhaps we only create rules to disallow particularly egregious dependencies/imports (e.g. jquery), or consider a more-scalable solution of having a single rule no-deprecated-imports with a list of disallowed imports that we add to during each major release.

As for @ember/render-modifiers, if this package is deprecated, shouldn't there be a more obvious deprecation warning in the readme and npm registry? Run npm deprecate ... to explicitly mark it as deprecated.

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Jun 30, 2023

I would love to mark that package as deprecated. I can start discussion towards that -- but regardless of deprecation status, it is _anti-pattern_y, and I think it would be good to maybe have a more generic import-controlling lint. 🤔 maybe avoid-libraries (or no-unpreferred-libraries)?
Each library could have a different reason for being avoided:

  • @ember/render-modifiers - anti-patterns, please use either derived data or a custom modifier
  • ember-cli-mirage - deprecated (soon), not yet (a new ember-mirage library is likely going to take its place, don't worry)
  • ... etc ...

@mansona
Copy link
Member

mansona commented Jun 30, 2023

@bmish I completely agree with this:

When we want to issue a community-level edict disallowing a particular dependency, I think at that point we may deserve to have an actual rule that we can document, test, that can be part of the recommended config

I understand that that might get a bit unwieldy to manage on our side but I actually thing it's much better for the end user if these are individual rules rather than creating a no-deprecated-imports rule. This way people could move off those imports gradually file-by-file using something like Lint to the future rather than adding ignores for all deprecated imports.

I think it also makes more sense in the case where something is in recommended that when you update a new rule is failing rather than a rule that you have already dealt with (and maybe even had a Jira ticket that was assigned to fix it 😂)

Anyway, this is all me just jumping in to say that I would personally prefer individual rules for deprecated imports rather than a single rule with a config list 👍

@bmish
Copy link
Member

bmish commented Jun 30, 2023

Sounds good, let's stick to a new rule for each deprecation.

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review July 1, 2023 01:39
@NullVoxPopuli
Copy link
Contributor Author

Ready for review!

docs/rules/no-at-ember-render-modifiers.md Show resolved Hide resolved
lib/rules/no-at-ember-render-modifiers.js Outdated Show resolved Hide resolved
tests/lib/rules/no-at-ember-render-modifiers.js Outdated Show resolved Hide resolved
@bmish bmish changed the title New Rule: no-render-modifiers Add new rule no-at-ember-render-modifiers Jul 6, 2023
Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New Rule Idea for a new lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Rule: no-render-modifiers
4 participants