Skip to content

feat: no-danger find props on all components (not just native) #3434

@AndersDJohnson

Description

@AndersDJohnson
Contributor

Unless I am mistaken, it seems today that the rule react/no-danger has specific logic to only find usages on native elements. See the isDOMComponent check here:

if (jsxUtil.isDOMComponent(node.parent) && isDangerous(node.name.name)) {

We think it could be useful to support non-native components as well, especially in cases like styled-components wrappers, or components that want to offer this prop to be forwarded internally to a native element.

This could be a new default behavior, or an opt-in behavior via a new rule option.

Activity

ljharb

ljharb commented on Sep 15, 2022

@ljharb
Member

Can you elaborate on why that would be useful?

A custom component can simply remove a dangerous prop, no?

AndersDJohnson

AndersDJohnson commented on Oct 1, 2022

@AndersDJohnson
ContributorAuthor

@ljharb You're totally right - a custom component could remove a dangerous prop. That's a good suggestion.

In practice, though, that's not already going to be in place, and may not be as feasible to roll out to large existing code bases, and especially not to roll out to existing 3rd party libraries where this may not be a proper assumption to bake-in, including older versions that consuming applications may be locked into and prevented from upgrading for now.

A side note, but I have something of a general philosophy around linting and tooling in general - and tend to believe that if it's going to be most effective and useful then it needs to support rolling out suggested patterns to existing code bases more incrementally, where lint tooling maintainers understand that some things cannot practically be fixed right away.

Anyway, the biggest practical example I have right now is styled-components (https://styled-components.com/docs/basics#getting-started):

import styled from 'styled-components';

// Create a Title component that'll render an <h1> tag with some styles
const Title = styled.h1`
  font-size: 1.5em;
  text-align: center;
  color: palevioletred;
`;

// ...

<Title dangerouslySetInnerHTML={{
  __html: '<span>Hello!</span>'
}} />

Example on CodeSandbox: https://codesandbox.io/s/styled-components-dangerous-26ugid

Screen Shot 2022-10-01 at 12 25 52 PM

Here you can immediately see how, although Title doesn't look like a native element syntactically (and according to ESLint AST), it still behaves almost exactly like one (other than the addition of styling - implicit className, etc.).
All native element props are made available on the styled wrapper and forwarded to the native element, including dangerouslySetInnerHTML.

But critically, in this case, we're not protected by the react/no-danger lint rule against using dangerouslySetInnerHTML,
because the rule today doesn't support detecting that prop on non-native elements, to my knowledge.

I don't know that styled-components would be able to support removing (or not forwarding) dangerouslySetInnerHTML as a general default, since I'm sure there are applications with different preferences or requirements that need to retain this behavior. Perhaps there would be a way for them to add a configuration option to control this, but I'm not sure that's the best approach.

So, I'm proposing a simple option to opt-in (or perhaps out) of having react/no-danger apply to non-native elements, too.

E.g.:

`.eslint

{
  "react/no-danger": ["error", {
    includeNonDOMElements: true
  }],
}

I'm happy to look into contributing an implementation, if the maintainers and/or community agree this would be a useful feature.

ljharb

ljharb commented on Oct 1, 2022

@ljharb
Member

I agree with you that styled-components - alone - might have a use case here, since they’re effectively a custom DSL for html components.

However, this plugin doesn’t support them, since they bear no static similarity to normal react components. I suggest locating a styled-components-specific eslint plugin which might have utilities for detecting those.

If you have a normal react use case, I’d be happy to consider reopening this, but i don’t think this plugin can help with your use case based on my understanding.

AndersDJohnson

AndersDJohnson commented on Oct 2, 2022

@AndersDJohnson
ContributorAuthor

@ljharb Understood. I think it would be a simple change to add an option to not run the jsxUtil.isDOMComponent(node.parent) check. But if there's no appetite for that, yes we can create a separate rule as a fork or whatever. Thanks for the discussion.

ljharb

ljharb commented on Oct 2, 2022

@ljharb
Member

Even with that, this plugin still can’t support styled-components since those aren’t normal components - they’re a tagged template literal DSL.

AndersDJohnson

AndersDJohnson commented on Oct 5, 2022

@AndersDJohnson
ContributorAuthor

@ljharb I think it's mostly relevant how styled-components are consumed, which is just like regular components, which are easy to target the way this rule is already doing.

ljharb

ljharb commented on Oct 5, 2022

@ljharb
Member

@AndersDJohnson ah hm, that's a fair point - it's about the jsx elements, which doesn't actually relate to the way the components are created.

So, that's a valid use case for adding an option that allows it to warn on custom components. Are there any others, or is that the only one?

reopened this on Oct 5, 2022
AndersDJohnson

AndersDJohnson commented on Oct 5, 2022

@AndersDJohnson
ContributorAuthor

@ljharb Exactly! Thanks for re-opening. I think that's it - just one generic boolean option to have it also target custom components / non-native JSX tags. I'd be willing to try to raise a PR to implement if I have time soon.

ljharb

ljharb commented on Oct 5, 2022

@ljharb
Member

Would it be better to explicitly list the names of components it should apply to, rather than a blanket "everything"?

AndersDJohnson

AndersDJohnson commented on Oct 5, 2022

@AndersDJohnson
ContributorAuthor

@ljharb That could be useful! I think we would still want to support a simple boolean, for projects that want to cover everything and not have to maintain a list, especially as new styled-components may be created often. But if we wanted, we could support optionally passing a string array of component names (instead of a boolean) to scope it more specifically.

ljharb

ljharb commented on Oct 5, 2022

@ljharb
Member

I suppose that makes sense.

ckcr4lyf

ckcr4lyf commented on Nov 4, 2022

@ckcr4lyf

+1 on this issue, had it come up with mui component <Box> which allows dangerouslySetInnerHTML, but this rule won't catch it (as it is under some other MUI components as well).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @ljharb@AndersDJohnson@ckcr4lyf

      Issue actions

        feat: no-danger find props on all components (not just native) · Issue #3434 · jsx-eslint/eslint-plugin-react