Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Promote nursery rules for 12.1.0 #4431

Merged
merged 1 commit into from
May 2, 2023
Merged

Promote nursery rules for 12.1.0 #4431

merged 1 commit into from
May 2, 2023

Conversation

Conaclos
Copy link
Contributor

@Conaclos Conaclos commented May 1, 2023

Summary

This PR promotes lint rules that were previously released in a stable version.

Promoted rules

A11y

  • noNoninteractiveElementToInteractiveRole
  • useAriaPropsForRole
  • useValidAriaProps
  • useValidLang
  • noRedundantAlt
  • noSvgWithoutTitle
  • useIframeTitle
  • useMediaCaption

Compelxity

I promoted the following rules because noExtraBooleanCast and noUselessFragments are part of compelxity:

  • noExtraSemicolons
  • noExtraLabels
  • noUselessCatch
  • noUselessConstructor
  • noUselessRename
  • noUselessSwitchCase

noExtraLabels should certainly be renamed to noUselessLabel?
I could rename noExtraSemicolons to noExtraSemicolon (without s because of noExtraBooleanCast).
noUselessSemicolon seems ambiguous because someone could argue that most of semicolons are useless.

I also promoted the following rule:

  • noWith

Correctness

I promoted the following rules that highlight errors:

  • noGlobalObjectCalls
  • noInvalidConstructorSuper
  • noUnreachableSuper
  • noUnsafeOptionalChaining
  • useYield

Note: noGlobalObjectCalls should certainly be moved to semantic_analyzers. However, I got an error when I moved the rule.

I promoted the following rules because noUnusedVariables is part of correctness:

  • noUnusedLabels

I also promoted the following rules because they prevent the access to undefined variables:

  • noInnerDeclarations
  • noSwitchDeclarations

Style

  • noCommaOperator
  • noInferrableTypes
  • noNamespace
  • noParameterAssign (not part of recommended)
  • noParameterProperties (not part of recommended)
  • noRestrictedGlobals (not part of recommended)

Suspicious

I promoted the following rules, because noDuplicate* are part of suspicious:

  • noDuplicateCase
  • noDuplicateClassMembers
  • noRedeclare

I promoted the following rules that highlight possible errors:

  • noAssignInExpressions
  • noClassAssign
  • noConfusingLabels
  • noPrototypeBuiltins (now recommended)
  • noSelfCompare

I also promoted the following rule in the group because module x {} is close to deprecation:

  • useNamespaceKeyword

Non-promoted rules

The following rules are still a WIP:

  • noSelfAssign
  • useExhaustiveDependencies
  • useHookAtTopLevel

I did not promote the following rules because I think they should be reworked in order to use the semantic model:

  • noBannedTypes
  • useIsNan

I did not promote the following rule because it should certainly be replaced in the future:

  • useCamelCase

Other changes

I updated the version of new rules to version 12.1.0.

Test Plan

Updated

Changelog

  • The PR requires a changelog line

Documentation

Updated.

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented May 1, 2023

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b322c0f
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/645119a33978e50007a31783
😎 Deploy Preview https://deploy-preview-4431--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Conaclos Conaclos changed the title Lint rule promotion Lint rule promotion 12.1.0 May 1, 2023
@github-actions github-actions bot added A-CLI Area: CLI A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Project Area: project configuration and loading labels May 1, 2023
@Conaclos Conaclos marked this pull request as draft May 1, 2023 16:23
@Conaclos Conaclos changed the title Lint rule promotion 12.1.0 Promote nursery rules for 12.1.0 May 1, 2023
@Conaclos Conaclos added this to the v12.1.0 milestone May 1, 2023
@Conaclos Conaclos requested a review from ematipico May 1, 2023 16:26
@Conaclos Conaclos marked this pull request as ready for review May 1, 2023 17:00
@ematipico
Copy link
Contributor

ematipico commented May 2, 2023

Thank you for looking after this!

Here's some feedback/questions:

  • noSelfAssign is not ready yet for promotion;
  • useNamespaceKeyword is conflicts (especially the code fix) with noNamespace. Should we have just one rule under suspicious? Especially because if modules are about to be deprecated;
  • can you show the error you got when you moved a rule?
  • useHookAtTopLevel is still a work in progress... We should take care of it;
  • noRedundantAlt , like all accessibility rules, should be recommended. They're important for accessibility! 😊
  • nursery rules, even when are recommended, they are not enabled in a production build. This means we are free to recommend them even when we promote them. We could recommend noPrototypeBuiltins
  • if you think useMediaCaption is not ready, maybe we should create an issue to track the work needed
  • useCamelCase, we could deprecate it. Although it would our first time, so I don't think we have in the website that shows a deprecated rule
  • useExhaustiveDependencies I'd like to wait too. At the moment I'm not very happy about the rule configuration. But the rule itself is stable. Surely not perfect, but we should wait that useHookAtTopLevel is ready too
  • noRestrictedGlobals should be ready

@Conaclos
Copy link
Contributor Author

Conaclos commented May 2, 2023

useNamespaceKeyword is conflicts (especially the code fix) with noNamespace. Should we have just one rule under suspicious? Especially because if modules are about to be deprecated

It is the reason I promoted noNamespace in style.
I first wanted to remove noNamespace from recommended and then noticed that noNamespace is part of ESLint recommended preset. useNamespaceKeyword is also part of the ESLint recommended preset.

Although the TypeScript devs do not recommend namespace x {} and module x {}, only module x {} is going to be deprecated in medium terms.

useCamelCase, we could deprecate it. Although it would our first time, so I don't think we have in the website that shows a deprecated rule

I could wait until we land an alternative rule.

if you think useMediaCaption is not ready, maybe we should create an issue to track the work needed

In light of your previous comments (about accessibility), the rule seems ready.
It just required more work to a user to respect it: transcription of all audios.

@ematipico
Copy link
Contributor

ematipico commented May 2, 2023

It is the reason I promoted noNamespace in style.
I first wanted to remove noNamespace from recommended and then noticed that noNamespace is part of ESLint recommended preset. useNamespaceKeyword is also part of the ESLint recommended preset.

That's the thing I don't understand. Let's suppose I enable both rules (let's forget about recommended sets for a moment):

  • useNamespaceKeyword will suggest a code fix, which we then apply;
  • then, noNamespace will trigger because the linter suggested something that... well, it's not valid for this rule?

I think it will become a poor DX from our end, to be honest. I don't know how's going to be the DX for ESLint, but here feels wrong. Am I overreacting? It's fine if I am 😅

@Conaclos
Copy link
Contributor Author

Conaclos commented May 2, 2023

@ematipico noNamespace rejects both namespace x {} and module x {}. Thus, noNamespace is triggered also before applying the fix of useNamespaceKeyword.

Do you think we should remove noNamespace from the recommended set?
Or useNamespaceKeyword?

@ematipico
Copy link
Contributor

ematipico commented May 2, 2023

@ematipico noNamespace rejects both namespace x {} and module x {}. Thus, noNamespace is triggered also before applying the fix of useNamespaceKeyword.

Do you think we should remove noNamespace from the recommended set? Or useNamespaceKeyword?

https://docs.rome.tools/playground/?indentStyle=space&quoteStyle=single&trailingComma=es5&semicolons=as-needed&lintRules=all&code=bQBvAGQAdQBsAGUAIABCACAAewB9AA%3D%3D

Both are triggered. Once we change module to namespace, we show a code action that conflicts with another rule.

Do you think we should remove noNamespace from the recommended set?
Or useNamespaceKeyword?

I am not familiar with the intent of these rules, and unfortunately, I don't know how's the DX of TS ESLint. From a point of view of a newbie, I would only recommend the rule that requests to change module to namespace.

@Conaclos
Copy link
Contributor Author

Conaclos commented May 2, 2023

I am not familiar with the intent of these rules, and unfortunately, I don't know how's the DX of TS ESLint. From a point of view of a newbie, I would only recommend the rule that requests to change module to namespace.

I could make the same move, since namespace are still used by the community. I will make the change.

@Conaclos
Copy link
Contributor Author

Conaclos commented May 2, 2023

I will also apply the following renaming if you agree:

  • rename noExtraLabels to noUselessLabel
  • rename noExtraSemicolons to noExtraSemicolon

@ematipico
Copy link
Contributor

Yeah, they definitely make sense! Sorry I forgot to mention it

@Conaclos Conaclos merged commit 3de13ce into rome:main May 2, 2023
@Conaclos Conaclos mentioned this pull request May 2, 2023
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Project Area: project configuration and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants