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

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

Closed
AndersDJohnson opened this issue Sep 15, 2022 · 12 comments · Fixed by #3748
Closed

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

AndersDJohnson opened this issue Sep 15, 2022 · 12 comments · Fixed by #3748

Comments

@AndersDJohnson
Copy link
Contributor

AndersDJohnson commented Sep 15, 2022

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.

@ljharb
Copy link
Member

ljharb commented Sep 15, 2022

Can you elaborate on why that would be useful?

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

@AndersDJohnson
Copy link
Contributor Author

@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
Copy link
Member

ljharb commented Oct 1, 2022

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.

@ljharb ljharb closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2022
@AndersDJohnson
Copy link
Contributor Author

@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
Copy link
Member

ljharb commented Oct 2, 2022

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
Copy link
Contributor Author

@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
Copy link
Member

ljharb commented Oct 5, 2022

@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?

@ljharb ljharb reopened this Oct 5, 2022
@AndersDJohnson
Copy link
Contributor Author

AndersDJohnson commented Oct 5, 2022

@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
Copy link
Member

ljharb commented Oct 5, 2022

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

@AndersDJohnson
Copy link
Contributor Author

@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
Copy link
Member

ljharb commented Oct 5, 2022

I suppose that makes sense.

@ckcr4lyf
Copy link

ckcr4lyf commented Nov 4, 2022

+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
Development

Successfully merging a pull request may close this issue.

3 participants