Add forbid-elements rule #890

kentor · 2016-10-06T04:33:43Z

May specify a list of forbidden elements and their desired replacement
elements.

addresses #887

ljharb · 2016-10-06T06:07:25Z

docs/rules/forbid-elements.md

+
+## Rule Details
+
+This rule checks all JSX components and verifies that no forbidden elements are used. When it detects a forbidden element, the error message will contain the specified elements that you should use instead. This rule is off by default. If on, no elements are forbidden by default.


this rule should also check React.createElement usages as well.

Sounds reasonable.

If we specify forbid: ['Modal'], then for which of these do we warn?

React.createElement('Modal'); React.createElement(Modal);

or with forbid: ['button'], which one?

React.createElement(button); React.createElement('button');

i think we should follow the same rules as JSX when translating names into either strings or identifiers

<Modal /> is the same as React.createElement(Modal), and I'd expect the same setting to forbid both.

I'm not sure about the latter example.

If we consider React.createElement calls that can be mapped back to JSX as "valid", then React.createElement('Modal'), and React.createElement(button) would not be valid.

<Modal /> translates to React.createElement(Modal), and there's no way that I know that we can get it to translate to React.createElement('Modal').

That's why I think this rule should not bother to make checks that cannot be mapped back to JSX, and can be left to a different rule that checks for createElement validity

I'm not sure we're on the same page here - <Modal /> is identical to React.createElement(Modal). React.createElement('Modal') is the same as <{'Modal'} /> which is simply invalid react/jsx.

In other words, if Button is forbidden, i could still do const NotButton = Button; <NotButton /> and work around this rule.

Sorry, made some typos earlier. I guess I am arguing for two things:

If 'button' is forbidden, I don't think this rule should stop someone from doing const button = 'div'; React.createElement(button) because that looks like that's done for a reason other than to circumvent the rule (otherwise they would just use eslint-ignore).

In general:

if 'button' is forbidden, then the rule should check for React.createElement('button') and ignore React.createElement(button)

if 'Modal' is forbidden, then the rule should check for React.createElement(Modal) and ignore React.createElement('Modal').

if 'a.b' is forbidden, then the rule should check for React.createElement(a.b) and ignore React.createElement('a.b').

if '_a' is forbidden, then the rule should check for React.createElement(_a) and ignore React.createElement('_a')

because this aligns with how React translates JSX element names into createElement calls

babel example: https://goo.gl/ALVt2s

I agree with your summary (although I'm pretty sure React.createElement won't work at all with strings that aren't DOM components)

ljharb · 2016-10-06T06:08:33Z

docs/rules/forbid-elements.md

+// [1, {forbid: ['button', 'input']}]
+<div><button /><input /></div>
+
+// [1, {forbid: [{button: 'Button']}]


i think maybe a better object schema would be:
{ component: 'button', replacement: 'Button', message: 'hey yo buttons suck, use Button' } - where "replacement" and "message" would both be optional.

That way you can use the object form for everything, and the string form is just a convenience.

I like this suggestion. The only thing is I don't want to use the word component because to me that means a react component which excludes simple DOM nodes. I like to use element because the it aligns with React.createElement()

oh sure, "element" makes more sense

ljharb · 2016-10-06T06:09:47Z

docs/rules/forbid-elements.md

+// [1, {forbid: [{button: 'Button']}]
+<button />
+
+// [1, {forbid: [{div: ['Box', 'View']}, 'button']]


I'm actually kind of confused what the point is of replacement elements is, unless it's to autofix or populate the message - but "message" would handle the latter, and an autofixer would need only a single replacement to be able to work. So, why multiple?

I don't personally have a use case of multiple replacements, but yeah your custom message suggestion would make specifying multiple replacements obsolete.

lencioni · 2016-10-06T16:09:31Z

docs/rules/forbid-elements.md

+// [1, {forbid: ['button']}]
+<Button />
+
+// [1, {forbid: [{button: 'Button']}]


Codebases may have multiple components with the same name. Would it be more useful to be able to specify a path to a component instead of or in addition to just a component name?

I think using a path would require using require.resolve combined with the file's dirname/process.cwd(), on both the provided path as well as the import/require path in the file, which might be dynamic.

I think that would be more robust, but I think there's already a lot of other rules that solely use the component name, so I think it'd be OK to keep with that pattern for now.

lencioni · 2016-10-06T16:11:06Z

lib/rules/forbid-elements.js

+      }
+
+      var last = replacements.pop();
+      return replacements.join(', ') + ', or ' + last;


Thanks for including the Oxford comma! A+++

kentor · 2016-10-07T05:41:21Z

@ljharb I've made the suggested schema changes. I've left out the auto fixing capability for now and I could add it in later.

kentor · 2017-01-30T01:51:38Z

I'm no longer interested in having this merged. If no one else wants this rule it's fine to close it imo.

ljharb · 2017-01-30T05:54:24Z

@kentor sorry this fell through the cracks - I still think it's a great rule to add.

ljharb

LGTM pending a fresh rebase

TheXardas · 2017-01-30T14:06:21Z

Will it work for named imports like:

import MyBlock from './Block.jsx';

class MyComponent extends React.Component {
    render() {
        return <MyBlock />;
    }
}

with forbidden Block element?

ljharb · 2017-01-30T16:16:09Z

@TheXardas no, there is no block element in that file.

TheXardas · 2017-01-30T19:13:33Z

But there is!
It is aliased as MyBlock, but it's still a Block component.

ljharb · 2017-01-30T21:15:41Z

@TheXardas right but there's no way static analysis can determine that within that file. The only way a linter rule can determine what kind of component a thing is is its name in the file. If someone intentionally names it MyBlock to get around the prohibition, there's nothing a linter rule could do about it (just like if they used an eslint override comment)

May specify a list of forbidden elements and a custom reporting message

kentor changed the title ~~Add new rule: forbid-elements~~ Add forbid-elements rule Oct 6, 2016

ljharb requested changes Oct 6, 2016

View reviewed changes

kentor force-pushed the forbid-elements branch from 8af0ff1 to 2978484 Compare October 6, 2016 06:10

lencioni reviewed Oct 6, 2016

View reviewed changes

kentor force-pushed the forbid-elements branch 2 times, most recently from 5efb73e to 3fd0df0 Compare October 7, 2016 05:37

yannickcr force-pushed the master branch from 5717b76 to 13806f1 Compare January 7, 2017 21:19

ljharb approved these changes Jan 30, 2017

View reviewed changes

ljharb added the new rule label Jan 30, 2017

kentor force-pushed the forbid-elements branch from 3fd0df0 to bbcbaee Compare January 31, 2017 04:14

Add new rule: forbid-elements

d884a98

May specify a list of forbidden elements and a custom reporting message

kentor force-pushed the forbid-elements branch from bbcbaee to d884a98 Compare January 31, 2017 07:13

lencioni merged commit ee144d6 into jsx-eslint:master Feb 15, 2017

renovate-bot mentioned this pull request Feb 16, 2017

Update dependency eslint-plugin-react to version 6.10.0 singapore/lint-condo#240

Merged

alexzherdev mentioned this pull request Jul 20, 2018

rule proposal: forbid element/components #887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add forbid-elements rule #890

Add forbid-elements rule #890

kentor commented Oct 6, 2016

ljharb Oct 6, 2016

kentor Oct 6, 2016

kentor Oct 6, 2016 •

edited

Loading

ljharb Oct 6, 2016

kentor Oct 7, 2016 •

edited

Loading

ljharb Oct 7, 2016

kentor Oct 7, 2016 •

edited

Loading

ljharb Oct 7, 2016

ljharb Oct 6, 2016

kentor Oct 6, 2016 •

edited

Loading

ljharb Oct 6, 2016

ljharb Oct 6, 2016

kentor Oct 6, 2016

lencioni Oct 6, 2016

ljharb Oct 6, 2016

lencioni Oct 6, 2016 •

edited

Loading

kentor commented Oct 7, 2016

kentor commented Jan 30, 2017

ljharb commented Jan 30, 2017

ljharb left a comment

TheXardas commented Jan 30, 2017

ljharb commented Jan 30, 2017

TheXardas commented Jan 30, 2017

ljharb commented Jan 30, 2017


		## Rule Details

		This rule checks all JSX components and verifies that no forbidden elements are used. When it detects a forbidden element, the error message will contain the specified elements that you should use instead. This rule is off by default. If on, no elements are forbidden by default.

Add forbid-elements rule #890

Add forbid-elements rule #890

Conversation

kentor commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentor Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentor Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentor Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentor Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

kentor commented Oct 7, 2016

kentor commented Jan 30, 2017

ljharb commented Jan 30, 2017

ljharb left a comment

Choose a reason for hiding this comment

TheXardas commented Jan 30, 2017

ljharb commented Jan 30, 2017

TheXardas commented Jan 30, 2017

ljharb commented Jan 30, 2017

kentor Oct 6, 2016 •

edited

Loading

kentor Oct 7, 2016 •

edited

Loading

kentor Oct 7, 2016 •

edited

Loading

kentor Oct 6, 2016 •

edited

Loading

lencioni Oct 6, 2016 •

edited

Loading