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: allow ignore components, sources #74

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

huynhducduy
Copy link
Contributor

@@ -53,6 +53,52 @@ With this configuration, the "style" attribute is ignored for native elements fo
}
```

As of v3.4.0, each eslint-plugin-react-perf rule supports configuration to control whether custom components are ignored.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remember to change the version (v3.4.0) to your desired one

@harshVardhan4743
Copy link

harshVardhan4743 commented Jan 6, 2025

Hi @huynhducduy , when can we expect to get it merged ? 🙏

@huynhducduy
Copy link
Contributor Author

Hi @huynhducduy , when can we expect to get it merged ? 🙏

@harshVardhan4743 Sorry but I don't know when @cvazac has the time to review it. At the mean time, you can try this PR and all of my other PRs by using this patch https://gist.github.com/huynhducduy/46515b884c583e9ad95a353829010638

options[0] || {};

const sourceMap = new Map();
const ignoreComponentsSet = new Set();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just do this:

const ignoreComponentsSet = new Set(ignoreComponents || []);

@cvazac
Copy link
Owner

cvazac commented Jan 8, 2025

Thanks for working on this @huynhducduy!

Today, we have nativeAllowList. Soon we will have allowList. For consistency, maybe ignoreComponents should be componentAllowList and ignoreImports should be importAllowList - wdyt?

{
code: `import Foo from "react-foo";<Foo.Item style={${ruleCode}} />`,
options: [
{ ignoreSources: [{ source: "react-foo", importNames: ["Item"] }] },
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to instead namespace as importNames: ["Foo.Item"]? wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to drop the source property?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to adjust the config to be able to, for example, allow the known patterns for Foo.Item, but disallow for Bar.Item.

import Foo from "react-foo"
import Bar from "react-bar"

render() {
  return <><Foo.Item /><Bar.Item /></>
}

The options in the PR on line 118 don't distinguish between Foo and Bar.

Copy link
Contributor Author

@huynhducduy huynhducduy Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I didn't get it yet. It does disallow for Bar.Item since Bar is imported from react-bar (not react-foo).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvazac have I answered all your questions? Is there anything else you are unclear about?

@huynhducduy
Copy link
Contributor Author

huynhducduy commented Jan 14, 2025

Thanks for working on this @huynhducduy!

Today, we have nativeAllowList. Soon we will have allowList. For consistency, maybe ignoreComponents should be componentAllowList and ignoreImports should be importAllowList - wdyt?

IMO, the allowList postfix will be used for props (allowList for React component's props, nativeAllowList for native component's props).
If we use that postfix, it can be misleading. That's why I choose ignore instead.

This is also my answer for #73 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable rule for react-router-dom <Route/> element.
3 participants