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

💅 lint/a11y/useAltText asks for alt when using object spread #3977

Open
1 task done
Tracked by #3727
gkiely opened this issue Sep 18, 2024 · 12 comments
Open
1 task done
Tracked by #3727

💅 lint/a11y/useAltText asks for alt when using object spread #3977

gkiely opened this issue Sep 18, 2024 · 12 comments
Assignees
Labels
A-Linter Area: linter S-Feature Status: new feature to implement
Milestone

Comments

@gkiely
Copy link

gkiely commented Sep 18, 2024

Environment information

CLI:
  Version:                      1.9.1
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_PATH:               unset
  BIOME_LOG_PREFIX_NAME:        unset
  BIOME_CONFIG_PATH:            unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.12.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/10.5.0"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    true
  VCS disabled:                 true

Linter:
  JavaScript enabled:           true
  JSON enabled:                 true
  CSS enabled:                  true
  GraphQL enabled:              false
  Recommended:                  true
  All:                          false
  Enabled rules:
  performance/noDelete
  suspicious/noCatchAssign
  suspicious/noUnsafeNegation
  complexity/useLiteralKeys
  complexity/noMultipleSpacesInRegularExpressionLiterals
  a11y/useValidLang
  complexity/noUselessEmptyExport
  suspicious/useNamespaceKeyword
  suspicious/useValidTypeof
  a11y/useValidAriaRole
  correctness/noConstantCondition
  a11y/useAriaActivedescendantWithTabindex
  suspicious/noDuplicateParameters
  style/useDefaultParameterLast
  complexity/noEmptyTypeParameters
  correctness/noConstructorReturn
  style/useSelfClosingElements
  suspicious/noDuplicateSelectorsKeyframeBlock
  correctness/noUnknownProperty
  correctness/noUnusedLabels
  complexity/noUselessTernary
  correctness/noUnreachableSuper
  suspicious/noCompareNegZero
  suspicious/noExplicitAny
  correctness/noSwitchDeclarations
  correctness/noUnsafeOptionalChaining
  correctness/noConstAssign
  suspicious/noControlCharactersInRegex
  complexity/noUselessTypeConstraint
  style/noVar
  suspicious/noDoubleEquals
  suspicious/noRedundantUseStrict
  style/useLiteralEnumMembers
  suspicious/noGlobalIsNan
  suspicious/noEmptyInterface
  suspicious/noConstEnum
  suspicious/noMisleadingCharacterClass
  correctness/noPrecisionLoss
  a11y/noLabelWithoutControl
  suspicious/noRedeclare
  correctness/noStringCaseMismatch
  correctness/noSetterReturn
  correctness/noInvalidConstructorSuper
  suspicious/noImplicitAnyLet
  suspicious/noDuplicateObjectKeys
  suspicious/noUnsafeDeclarationMerging
  correctness/noUnreachable
  suspicious/noFallthroughSwitchClause
  complexity/noUselessThisAlias
  complexity/useOptionalChain
  correctness/noInnerDeclarations
  style/noParameterAssign
  suspicious/noDuplicateCase
  complexity/useRegexLiterals
  correctness/noSelfAssign
  correctness/noInvalidBuiltinInstantiation
  style/noUselessElse
  style/useShorthandFunctionType
  suspicious/noShadowRestrictedNames
  correctness/noInvalidDirectionInLinearGradient
  suspicious/noImportantInKeyframe
  complexity/noUselessLabel
  complexity/noUselessCatch
  correctness/noUnsafeFinally
  a11y/useAriaPropsForRole
  correctness/noNonoctalDecimalEscape
  style/useEnumInitializers
  a11y/useHtmlLang
  suspicious/noDuplicateTestHooks
  style/useWhile
  complexity/useArrowFunction
  style/noInferrableTypes
  a11y/noNoninteractiveTabindex
  complexity/useSimpleNumberKeys
  correctness/useYield
  a11y/noInteractiveElementToNoninteractiveRole
  style/useNumericLiterals
  correctness/noUnnecessaryContinue
  suspicious/noApproximativeNumericConstant
  suspicious/noImportAssign
  suspicious/noLabelVar
  correctness/noGlobalObjectCalls
  suspicious/useDefaultSwitchClauseLast
  correctness/noUnknownUnit
  a11y/useAltText
  performance/noBarrelFile
  correctness/noEmptyCharacterClassInRegex
  a11y/useIframeTitle
  complexity/noBannedTypes
  a11y/noSvgWithoutTitle
  correctness/noVoidElementsWithChildren
  style/useAsConstAssertion
  correctness/useJsxKeyInIterable
  style/useExportType
  style/noArguments
  suspicious/noDebugger
  a11y/useValidAriaValues
  suspicious/noMisleadingInstantiator
  suspicious/noCommentText
  a11y/useFocusableInteractive
  correctness/noUnmatchableAnbSelector
  suspicious/noGlobalAssign
  suspicious/noDuplicateJsxProps
  suspicious/noThenProperty
  a11y/noPositiveTabindex
  correctness/noEmptyPattern
  complexity/noExcessiveNestedTestSuites
  security/noDangerouslySetInnerHtmlWithChildren
  suspicious/noExtraNonNullAssertion
  suspicious/noShorthandPropertyOverrides
  suspicious/useGetterReturn
  correctness/noRenderReturnValue
  security/noGlobalEval
  suspicious/noPrototypeBuiltins
  style/noNonNullAssertion
  a11y/noRedundantRoles
  complexity/useFlatMap
  correctness/useIsNan
  suspicious/noGlobalIsFinite
  suspicious/noSelfCompare
  suspicious/noAsyncPromiseExecutor
  suspicious/noDuplicateFontNames
  style/useNodejsImportProtocol
  a11y/noDistractingElements
  complexity/noWith
  suspicious/noDuplicateClassMembers
  complexity/noExtraBooleanCast
  performance/noAccumulatingSpread
  a11y/useValidAriaProps
  a11y/noRedundantAlt
  correctness/noChildrenProp
  correctness/noUnknownFunction
  correctness/noInvalidPositionAtImportRule
  suspicious/noConfusingLabels
  a11y/useButtonType
  a11y/useSemanticElements
  a11y/noAriaUnsupportedElements
  correctness/noInvalidGridAreas
  correctness/noFlatMapIdentity
  suspicious/noSuspiciousSemicolonInJsx
  suspicious/noSparseArray
  a11y/useHeadingContent
  correctness/useValidForDirection
  correctness/noVoidTypeReturn
  correctness/noInvalidUseBeforeDeclaration
  a11y/noAriaHiddenOnFocusable
  a11y/useGenericFontNames
  correctness/noUnknownMediaFeatureName
  a11y/useAnchorContent
  complexity/noUselessRename
  complexity/noUselessConstructor
  a11y/noAccessKey
  style/useExponentiationOperator
  complexity/noUselessSwitchCase
  style/useSingleVarDeclarator
  suspicious/noExportsInTest
  a11y/noNoninteractiveElementToInteractiveRole
  style/noCommaOperator
  suspicious/noDuplicateAtImportRules
  suspicious/useIsArray
  a11y/noHeaderScope
  suspicious/noMisrefactoredShorthandAssign
  suspicious/noEmptyBlock
  suspicious/noClassAssign
  suspicious/noFunctionAssign

Rule name

lint/a11y/useAltText

Playground link

https://biomejs.dev/playground/?code=YwBvAG4AcwB0ACAAZgBsAGEAZwAgAD0AIAB0AHIAdQBlADsACgAKAGMAbwBuAHMAdAAgAHQAZQBzAHQAIAA9ACAAKAApACAAPQA%2BACAAewAKACAAIAByAGUAdAB1AHIAbgAgADwAaQBtAGcACgAgACAAIAAgAGEAbAB0AD0AIgBhAGwAdAAgAHQAYQBnACIACgAgACAAIAAgAHMAcgBjAD0AIgAiAAoAIAAgACAAIAB7AC4ALgAuACgAZgBsAGEAZwAgACYAJgAgAHsACgAgACAAIAAgACAAIAByAGUAZgBlAHIAcgBlAHIAUABvAGwAaQBjAHkAOgAgACcAbgBvAC0AcgBlAGYAZQByAHIAZQByACcALAAKACAAIAAgACAAfQApAH0ACgAgACAALwA%2BAAoAfQA%3D

Expected result

Should not show an error as the alt tag is included

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@ematipico
Copy link
Member

Duplicate of #3660

@ematipico ematipico marked this as a duplicate of #3660 Sep 18, 2024
@ematipico ematipico closed this as not planned Won't fix, can't repro, duplicate, stale Sep 18, 2024
@gkiely
Copy link
Author

gkiely commented Sep 18, 2024

Biome doesn't know if the props contain a prop that could pass an incorrect value for alt, so it's more conservative and it raises it as an issue.

@ematipico is there any interest in a flag in biome that says "if an alt tag is present in the component props it doesn't error and it is up to the developer to manage dynamic props".

tbh that is what I thought the default behavior would be.

@Netail
Copy link
Contributor

Netail commented Oct 1, 2024

Facing the same issue, while I specifically add a fallback text when the component alt prop is empty or nullish;
Playground

@ematipico
Copy link
Member

Biome doesn't know if the props contain a prop that could pass an incorrect value for alt, so it's more conservative and it raises it as an issue.

@ematipico is there any interest in a flag in biome that says "if an alt tag is present in the component props it doesn't error and it is up to the developer to manage dynamic props".

tbh that is what I thought the default behavior would be.

Sure, I don't see a particular issue with that, however I want to clearly understand a couple of real use cases, so we can document properly the new option.

@Conaclos
Copy link
Member

Conaclos commented Oct 1, 2024

@gkiely @Netail Is there a reason why you put the spread after setting the alt and src attributes?

We could improve the diagnostic saying that this should be flipped.
Otherwise, we could simply accept this code. I am not fond of adding an option just for that...

@ematipico
Copy link
Member

@gkiely @Netail Is there a reason why you put the spread after setting the alt and src attributes?

We could improve the diagnostic saying that this should be flipped. Otherwise, we could simply accept this code. I am not fond of adding an option just for that...

There's a veeeery old discussion in the old Rome Tools repository. This is a pattern that is usually used in component libraries.

@Netail
Copy link
Contributor

Netail commented Oct 1, 2024

@gkiely @Netail Is there a reason why you put the spread after setting the alt and src attributes?

We could improve the diagnostic saying that this should be flipped. Otherwise, we could simply accept this code. I am not fond of adding an option just for that...

No particular reason in terms of functionality I am using, I use it mainly for style preference.

When you create a rest prop variable, it always has to be at the end, so I always use the spread operator at the end as well. It's also a piece of uninteresting code, so when you read a component from left to right you want the most important props mentioned first.

I can see a use-case where people use the spread-operator for overwriting default values I guess. Let's say the rest prop variable contains an alt attribute, but you also specify a custom alt text before the spread operator, the spread operator can override the manually set prop.

@Conaclos
Copy link
Member

Conaclos commented Oct 1, 2024

Ok I see. I think we can simply check if alt exists and ignore the spread. This looks fair to me.

@Netail
Copy link
Contributor

Netail commented Oct 1, 2024

Ok I see. I think we can simply check if alt exists and ignore the spread. This looks fair to me.

That would be lovely, maybe nice to reopen this issue until this is implemented? Easier to keep track of when this has been resolved

@Conaclos Conaclos reopened this Oct 1, 2024
@Conaclos
Copy link
Member

Conaclos commented Oct 1, 2024

If others accept, we can accept a PR for that.

@ematipico
Copy link
Member

Ok I see. I think we can simply check if alt exists and ignore the spread. This looks fair to me.

Some rules use the same logic, this means we should change it for all of them.

I agree with the change, however:

  • I see it as a breaking change because we now decide to catch less cases. It's possible other users rely on it.
  • Other rules need to align too.

I would close this issue, and open a new issue that explains the work to do.

@ematipico ematipico added this to the Biome 2.0 milestone Oct 7, 2024
@ematipico ematipico added A-Linter Area: linter S-Feature Status: new feature to implement labels Oct 7, 2024
@fireairforce fireairforce self-assigned this Jan 21, 2025
@fireairforce
Copy link
Member

i will fix this, it sounds like not very hard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter S-Feature Status: new feature to implement
Projects
None yet
Development

No branches or pull requests

5 participants