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

Jump to bottom

react/jsx-no-undef allowGlobals option #1013

Merged
merged 1 commit into from Apr 23, 2017
Merged

react/jsx-no-undef allowGlobals option #1013

merged 1 commit into from Apr 23, 2017

Conversation

jomasti
Copy link
Contributor

@jomasti jomasti commented Jan 5, 2017 •
edited

This should allow for the use case outlined in #922.

With the disallowGlobals option enabled, the global scope will be ignored
when checking for defined components. Without this option enabled,
Pascal case global variables will be allowed when checking for defined
components.

@jomasti @ljharb
[breaking] Add allowGlobals option for jsx-no-undef rule
dc3a643 
With the `allowGlobals` enabled, the global scope will be ignored when
checking for defined components. Without this option enabled, global
variables will not be allowed when checking for defined components.
ljharb
ljharb requested changes Jan 5, 2017
View reviewed changes
tests/lib/rules/display-name.js Outdated
}]
}],
globals: {
Foo: true
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary? i wouldn't expect the display-name tests to enable jsx-no-undef

tests/lib/rules/jsx-no-undef.js Outdated
@@ -56,6 +56,29 @@ ruleTester.run('jsx-no-undef', rule, {
'}'
].join('\n'),
parserOptions: parserOptions
}, {
code: '/*eslint no-undef:1*/ var React; React.render(<Text />);',
Copy link
Member

Choose a reason for hiding this comment

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

similarly, why would the core undef rule be enabled for jsx-no-undef tests? Rule tests should start with zero rules enabled except for the rule under test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the tests that were already there. I removed it.

tests/lib/rules/jsx-no-undef.js Outdated
'};'
].join('\n'),
errors: [{
message: '\'Text\' is not defined.'
Copy link
Member

Choose a reason for hiding this comment

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

Why is there an "errors" section in the "valid" section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy and paste. I must have missed that section when I blinked while scrolling.

@jomasti
Copy link
Contributor Author

jomasti commented Jan 5, 2017

Also, maybe the name of the option should be changed since it's not really just checking imports. It's checking the module scope. moduleOnly or something like that?

lencioni
lencioni approved these changes Feb 15, 2017
View reviewed changes
Copy link
Collaborator

@lencioni lencioni left a comment

Choose a reason for hiding this comment

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

Regarding the option name, perhaps disallowGlobals or something like that would be more accurate?

docs/rules/jsx-no-undef.md Outdated

The following patterns are considered okay and do not cause warnings:

```js
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this should be jsx (here and elsewhere)

docs/rules/jsx-no-undef.md Outdated

```js
...
"jsx-no-undef": [<enabled>, { "importedOnly": <boolean> }]
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: In #1071 we are adding react/ to lines like this. It would be nice to get that in here if we touch this again.

docs/rules/jsx-no-undef.md Outdated
...
```

### `importedOnly`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why make this an option instead of having it be the default behavior?

Copy link
Member

Choose a reason for hiding this comment

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

That seems like it'd be a breaking change - an option now, flipping the default later, seems safer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that makes sense. I'll open an issue for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#1072

@lencioni lencioni mentioned this pull request Feb 15, 2017
Closed
@jomasti jomasti force-pushed the jsx-no-undef-imported-only branch 2 times, most recently from a02db45 to 0ecd498 Compare February 16, 2017 03:43
@jomasti jomasti changed the title Jsx no undef imported only react/jsx-no-undef disallowGlobals option Feb 16, 2017
@ljharb ljharb added this to the 7.0.0 milestone Apr 23, 2017
@ljharb ljharb force-pushed the jsx-no-undef-imported-only branch from 0ecd498 to dc3a643 Compare April 23, 2017 18:43
@ljharb ljharb changed the title react/jsx-no-undef disallowGlobals option react/jsx-no-undef allowGlobals option Apr 23, 2017
ljharb
ljharb approved these changes Apr 23, 2017
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I've updated the PR to switch the option to allowGlobals, and default it to false.

@ljharb ljharb self-assigned this Apr 23, 2017
@ljharb
Copy link
Member

ljharb commented Apr 23, 2017

@jomasti @lencioni any objections to this being merged with my latest changes?

@lencioni
Copy link
Collaborator

@ljharb go for it

@ljharb ljharb merged commit ed928c2 into jsx-eslint:master Apr 23, 2017
@jomasti jomasti deleted the jsx-no-undef-imported-only branch November 11, 2017 16:57
@renovate renovate bot mentioned this pull request Feb 2, 2018
Open
1 task
@alexzherdev alexzherdev mentioned this pull request Jul 3, 2018
Closed
This was referenced Sep 22, 2018
Closed
Closed
Open
Open
Open
This was referenced Oct 25, 2018
Merged
Open
@renovate renovate bot mentioned this pull request Nov 7, 2018
Merged
@renovate renovate bot mentioned this pull request Nov 23, 2018
Open
1 task
@renovate renovate bot mentioned this pull request Dec 5, 2018
Closed
1 task
@mesosphere-web mesosphere-web mentioned this pull request Dec 19, 2018
Closed
1 task
github-actions bot pushed a commit to capitnflam/eslint-plugin that referenced this pull request Mar 16, 2024
@semantic-release-bot
release(version): Release 1.1.0 [skip ci]
bf7c725 
# [1.1.0](v1.0.1...v1.1.0) (2024-03-16)

### chore

* **deps-dev:** bump [@types](https://github.com/types)/node from 20.11.27 to 20.11.28 ([#13](#13)) ([738e28c](738e28c))
* **deps:** bump eslint-plugin-react from 7.34.0 to 7.34.1 ([#12](#12)) ([ad9f1b3](ad9f1b3)), closes [#3700](https://github.com/capitnflam/eslint-plugin/issues/3700) [#3701](https://github.com/capitnflam/eslint-plugin/issues/3701) [#3704](https://github.com/capitnflam/eslint-plugin/issues/3704) [#3705](https://github.com/capitnflam/eslint-plugin/issues/3705) [#3707](https://github.com/capitnflam/eslint-plugin/issues/3707) [#3713](https://github.com/capitnflam/eslint-plugin/issues/3713) [#3715](https://github.com/capitnflam/eslint-plugin/issues/3715) [#1000](https://github.com/capitnflam/eslint-plugin/issues/1000) [jsx-eslint/eslint-plugin-react#1000](jsx-eslint/eslint-plugin-react#1000) [#1002](https://github.com/capitnflam/eslint-plugin/issues/1002) [jsx-eslint/eslint-plugin-react#1002](jsx-eslint/eslint-plugin-react#1002) [#1005](https://github.com/capitnflam/eslint-plugin/issues/1005) [jsx-eslint/eslint-plugin-react#1005](jsx-eslint/eslint-plugin-react#1005) [#100](https://github.com/capitnflam/eslint-plugin/issues/100) [jsx-eslint/eslint-plugin-react#100](jsx-eslint/eslint-plugin-react#100) [#1010](https://github.com/capitnflam/eslint-plugin/issues/1010) [jsx-eslint/eslint-plugin-react#1010](jsx-eslint/eslint-plugin-react#1010) [#1013](https://github.com/capitnflam/eslint-plugin/issues/1013) [jsx-eslint/eslint-plugin-react#1013](jsx-eslint/eslint-plugin-react#1013) [#1022](https://github.com/capitnflam/eslint-plugin/issues/1022) [jsx-eslint/eslint-plugin-react#1022](jsx-eslint/eslint-plugin-react#1022) [#1029](https://github.com/capitnflam/eslint-plugin/issues/1029) [jsx-eslint/eslint-plugin-react#1029](jsx-eslint/eslint-plugin-react#1029) [#102](https://github.com/capitnflam/eslint-plugin/issues/102) [jsx-eslint/eslint-plugin-react#102](jsx-eslint/eslint-plugin-react#102) [#1034](https://github.com/capitnflam/eslint-plugin/issues/1034) [jsx-eslint/eslint-plugin-react#1034](jsx-eslint/eslint-plugin-react#1034) [#1038](https://github.com/capitnflam/eslint-plugin/issues/1038) [jsx-eslint/eslint-plugin-react#1038](jsx-eslint/eslint-plugin-react#1038) [#1041](https://github.com/capitnflam/eslint-plugin/issues/1041) [jsx-eslint/eslint-plugin-react#1041](jsx-eslint/eslint-plugin-react#1041) [#1043](https://github.com/capitnflam/eslint-plugin/issues/1043) [jsx-eslint/eslint-plugin-react#1043](jsx-eslint/eslint-plugin-react#1043) [#1046](https://github.com/capitnflam/eslint-plugin/issues/1046) [jsx-eslint/eslint-plugin-react#1046](jsx-eslint/eslint-plugin-react#1046) [#1047](https://github.com/capitnflam/eslint-plugin/issues/1047) [jsx-eslint/eslint-plugin-react#1047](jsx-eslint/eslint-plugin-react#1047) [#1050](https://github.com/capitnflam/eslint-plugin/issues/1050) [jsx-eslint/eslint-plugin-react#1050](jsx-eslint/eslint-plugin-react#1050) [#1053](https://github.com/capitnflam/eslint-plugin/issues/1053) [jsx-eslint/eslint-plugin-react#1053](jsx-eslint/eslint-plugin-react#1053) [#1057](https://github.com/capitnflam/eslint-plugin/issues/1057) [jsx-eslint/eslint-plugin-react#1057](jsx-eslint/eslint-plugin-react#1057) [#105](https://github.com/capitnflam/eslint-plugin/issues/105) [jsx-eslint/eslint-plugin-react#105](jsx-eslint/eslint-plugin-react#105) [#1061](https://github.com/capitnflam/eslint-plugin/issues/1061) [jsx-eslint/eslint-plugin-react#1061](jsx-eslint/eslint-plugin-react#1061) [#1062](https://github.com/capitnflam/eslint-plugin/issues/1062) [jsx-eslint/eslint-plugin-react#1062](jsx-eslint/eslint-plugin-react#1062) [#1070](https://github.com/capitnflam/eslint-plugin/issues/1070) [jsx-eslint/eslint-plugin-react#1070](jsx-eslint/eslint-plugin-react#1070) [#1071](https://github.com/capitnflam/eslint-plugin/issues/1071) [jsx-eslint/eslint-plugin-react#1071](jsx-eslint/eslint-plugin-react#1071) [#1073](https://github.com/capitnflam/eslint-plugin/issues/1073) [jsx-eslint/eslint-plugin-react#1073](jsx-eslint/eslint-plugin-react#1073) [#1076](https://github.com/capitnflam/eslint-plugin/issues/1076) [jsx-eslint/eslint-plugin-react#1076](jsx-eslint/eslint-plugin-react#1076) [#1079](https://github.com/capitnflam/eslint-plugin/issues/1079) [jsx-eslint/eslint-plugin-react#1079](jsx-eslint/eslint-plugin-react#1079) [#1088](https://github.com/capitnflam/eslint-plugin/issues/1088) [jsx-eslint/eslint-plugin-react#1088](jsx-eslint/eslint-plugin-react#1088) [#1098](https://github.com/capitnflam/eslint-plugin/issues/1098) [jsx-eslint/eslint-plugin-react#1098](jsx-eslint/eslint-plugin-react#1098) [#1101](https://github.com/capitnflam/eslint-plugin/issues/1101) [jsx-eslint/eslint-plugin-react#1101](jsx-eslint/eslint-plugin-react#1101) [#1103](https://github.com/capitnflam/eslint-plugin/issues/1103) [jsx-eslint/eslint-plugin-react#1103](jsx-eslint/eslint-plugin-react#1103) [#110](https://github.com/capitnflam/eslint-plugin/issues/110) [jsx-eslint/eslint-plugin-react#110](jsx-eslint/eslint-plugin-react#110) [#1116](https://github.com/capitnflam/eslint-plugin/issues/1116) [jsx-eslint/eslint-plugin-react#1116](jsx-eslint/eslint-plugin-react#1116) [#1117](https://github.com/capitnflam/eslint-plugin/issues/1117) [jsx-eslint/eslint-plugin-react#1117](jsx-eslint/eslint-plugin-react#1117) [#1119](https://github.com/capitnflam/eslint-plugin/issues/1119) [jsx-eslint/eslint-plugin-react#1119](jsx-eslint/eslint-plugin-react#1119) [#1121](https://github.com/capitnflam/eslint-plugin/issues/1121) [jsx-eslint/eslint-plugin-react#1121](jsx-eslint/eslint-plugin-react#1121) [#1122](https://github.com/capitnflam/eslint-plugin/issues/1122) [jsx-eslint/eslint-plugin-react#1122](jsx-eslint/eslint-plugin-react#1122) [#1123](https://github.com/capitnflam/eslint-plugin/issues/1123) [jsx-eslint/eslint-plugin-react#1123](jsx-eslint/eslint-plugin-react#1123) [#3700](https://github.com/capitnflam/eslint-plugin/issues/3700) [#3701](https://github.com/capitnflam/eslint-plugin/issues/3701) [#3704](https://github.com/capitnflam/eslint-plugin/issues/3704) [#3705](https://github.com/capitnflam/eslint-plugin/issues/3705) [#3707](https://github.com/capitnflam/eslint-plugin/issues/3707) [#3713](https://github.com/capitnflam/eslint-plugin/issues/3713) [#3715](https://github.com/capitnflam/eslint-plugin/issues/3715) [#3715](https://github.com/capitnflam/eslint-plugin/issues/3715) [jsx-eslint/eslint-plugin-react#3715](jsx-eslint/eslint-plugin-react#3715) [#3713](https://github.com/capitnflam/eslint-plugin/issues/3713) [jsx-eslint/eslint-plugin-react#3713](jsx-eslint/eslint-plugin-react#3713) [#3707](https://github.com/capitnflam/eslint-plugin/issues/3707) [jsx-eslint/eslint-plugin-react#3707](jsx-eslint/eslint-plugin-react#3707) [#3705](https://github.com/capitnflam/eslint-plugin/issues/3705) [jsx-eslint/eslint-plugin-react#3705](jsx-eslint/eslint-plugin-react#3705) [#3704](https://github.com/capitnflam/eslint-plugin/issues/3704) [jsx-eslint/eslint-plugin-react#3704](jsx-eslint/eslint-plugin-react#3704) [#3701](https://github.com/capitnflam/eslint-plugin/issues/3701) [jsx-eslint/eslint-plugin-react#3701](jsx-eslint/eslint-plugin-react#3701) [#3700](https://github.com/capitnflam/eslint-plugin/issues/3700) [jsx-eslint/eslint-plugin-react#3700](jsx-eslint/eslint-plugin-react#3700)

### ci

* add auto assign action ([#14](#14)) ([07d1f9a](07d1f9a))
* add check workflow ([#11](#11)) ([8afc82a](8afc82a))

### feat

* add [@eslint-community](https://github.com/eslint-community)/eslint-plugin-eslint-comments ([#17](#17)) ([fe2bf30](fe2bf30))
* add eslint-plugin-n ([#18](#18)) ([203d603](203d603))
* add eslint-plugin-security ([#16](#16)) ([e7f8c2e](e7f8c2e))
* add eslint-plugin-sonarjs ([#15](#15)) ([5bca4e1](5bca4e1))
* **react:** add some security linting ([#10](#10)) ([4424b67](4424b67))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lencioni lencioni lencioni approved these changes

@ljharb ljharb ljharb approved these changes

Assignees

@ljharb ljharb

Labels
enhancement new rule
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jomasti @ljharb @lencioni