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

react/jsx-no-undef allowGlobals option #1013

Merged
merged 1 commit into from Apr 23, 2017

Conversation

jomasti
Copy link
Contributor

@jomasti jomasti commented Jan 5, 2017

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.

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.
}]
}],
globals: {
Foo: true
Copy link
Member

@ljharb ljharb Jan 5, 2017

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

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

@ljharb ljharb Jan 5, 2017

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

@jomasti jomasti Jan 5, 2017

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.

'};'
].join('\n'),
errors: [{
message: '\'Text\' is not defined.'
Copy link
Member

@ljharb ljharb Jan 5, 2017

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

@jomasti jomasti Jan 5, 2017

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?

Copy link
Collaborator

@lencioni lencioni left a comment

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


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

```js
Copy link
Collaborator

@lencioni lencioni Feb 15, 2017

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)


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

@lencioni lencioni Feb 15, 2017

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.

...
```

### `importedOnly`
Copy link
Collaborator

@lencioni lencioni Feb 15, 2017

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

@ljharb ljharb Feb 15, 2017

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

@lencioni lencioni Feb 15, 2017

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

@lencioni lencioni Feb 15, 2017

Choose a reason for hiding this comment

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

@jomasti jomasti force-pushed the jsx-no-undef-imported-only branch 2 times, most recently from a02db45 to 0ecd498 Compare Feb 16, 2017
@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 Apr 23, 2017
@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
Copy link
Member

@ljharb ljharb left a comment

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

lencioni commented Apr 23, 2017

@ljharb go for it

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

Successfully merging this pull request may close these issues.

None yet

3 participants