Allow to configure implicitly required variables for JSX identifiers via pluggable API

[gaearon]

I'm not a parser person so forgive if my terminology is off.

I propose #1867 not be ignored, but solved in a way that is not specific for React. To remind you, the original issue is React's JSX requiring React to be in scope:

var React = require('react'); // ESLint thinks it's unused, but in in fact *is* used (and needed!)
var Table = require('Table.jsx');

module.exports = (
  <Table />
);

actually compiles to

var React = require('react');
var Table = require('Table.jsx');

module.exports = (
  React.createElement(Table, null) // Because this will actually will use React
);

I understand maintainers not wanting to special-case React in any way and I agree it's sensible. (Other libs may want to adopt JSX for their purposes.)

What I propose is making it possible (via custom parser like babel-eslint, or via some other extension point provided by ESLint) for consumer to specify which variables are implicitly required by JSX identifiers.

For example, I might have config like

{
  "parser": "babel-eslint",
  "jsx-target": "react-eslint" // I'm bad at naming things lol
}

Where react-eslint would be a package that, if specified, tells ESLint that any use of JSX identifier is equivalent to using React in that scope.

[gaearon]

cc @sebmck @nzakas who know much better than me how to implement this API-wise.

[gaearon]

I wonder if it can be made even more generic so tools like Babel can also use such package (e.g. called jsx-react, jsx-virtual-dom) whose sole function is helping the tools figure out how JSX translates to function calls. Relevant Babel issue: babel/babel#910

[glenjamin]

Some notes in reply to babel/babel-eslint#6 (comment)

The complexity here is that React is not set globally, it's being created locally (the top-level scope in browserified code is a function scope, not the global scope). You can already specify some globals that don't have to be used in the .eslintrc file. By including it specifically, you are telling ESLint, "hey, this is a local variable that I'm planning on using."

100% agreed that it's annoying that React makes us do this.
The eslint config allows specifying variables as global - and this means variables won't warn on undef or unused? That's handy.

I've already shown how to disable this warning. I'm not sure why there's resistance to that. Any other type of "special configuration" would also be extra code somewhere. Why not use what's already available?

The main reason for wanting something more is that we don't want to make React an actual global variable. therefore using React's JSX syntax in a file without also requiring React is an error. Yes this sucks that you must require react in scope to use JSX - but that's why we'd like the linter to catch it! :)

There's a few reasons for not wanting to put React on global, facebook/react#3252 is one example.

To me, the real change here is that the JSX compiler/transpiler should be inserting the React reference for you as part of that step. Forcing you to remember to do that seems like unnecessary cognitive overhead.

This would also be a sensible way to do it, but as it currently stands the compiler doesn't have knowledge of the module system so I don't think this is possible :(

There is a proposal open that could mean React isn't required to be in scope anymore - but even that would only apply in production mode.

Perhaps @sebmarkbage could weigh in?

As for some sort of pluggable JSX extension module, I guess using the Plugin API would be doable? http://eslint.org/docs/developer-guide/working-with-plugins.html @gaearon

[nzakas]

An additional thing that makes this tricky is that we're not talking about exposing a declaration, we're talking about exposing a reference (or rather, inferring a reference). escope is used to track references to variables, and that's not exposed in plugins. It's technically possible to expose it, but then we'd need to figure out an interface that makes sense, and that's not very straightforward.

Just so I'm being super clear: I'm completely open to creating something that allows the functionality you want. I just want it to be library-independent.

And to be completely transparent: my fear is that such an effort would be quite large as opposed to finding a way outside of ESLint to do it. For instance, why not add the React reference yourself as part of a build system rather than writing it manually? You could basically do that as:

echo 'var React = require("react");' | cat - yourfile.js > temp && mv temp yourfile.js

Since you already need to run your code through a compilation step, adding something like this wouldn't interfere with developer workflow and would keep all concerns nicely separated.

[gaearon]

For instance, why not add the React reference yourself as part of a build system rather than writing it manually?

I understand your concerns above.
As for my concerns with what you suggested, I wouldn't do that for a number of reasons:

• Each transformation has a cost. It's not like you need to apply it to one file; you need to do that for every app module. There's a performance/memory cost with adding more transformations when we're talking 1000+ modules.
• What if developer actually imports React on their own in the same file? It will be redefined. Not a biggie, but might cause potential issues if developer imports it in a different way, and different files may get different React copies (e.g. global vs NPM). And this wouldn't be visible from code unless you look at compiled output.
• What if they try to use React.addons which is only exposed if you require('react/addons')? Having “built-in” React that doesn't have addons would surely trip them, but some people don't use addons so it's wrong to automatically import react/addons either.
• What should it output? require? What if I want to switch to ES6 modules or another build system? I'd have to port my transformation. It's not difficult, but it's still a maintenance cost.
• It might be okay for one project, but what about open source code? Libraries with examples? I want to lint my open source libraries but I don't want to drag additional build transformations into them to appease the linter. People using example for the libraries want to be able to run or copy/paste these examples with no transformations (or with standard transformations such as “you'll need ES6 transpiler”). Adding something else to the mix causes confusion and potential issues.

I like this option even less than adding “ignore” comments because it changes not just the code, but its semantics, and may cause subtle misunderstandings, bugs and maintenance cost.

[gaearon]

If it's hard to do and scope info is not currently exposed, I guess we'll have to stick with the comments.

[nzakas]

Interesting, your response actually makes me think that using a comment to opt out is the best way forward. If you have different usage patterns such that you can't just add the line automatically, that probably means you should be explicitly flagging this case. If you are importing React in a different spot, or accessing React.addons, then you'd want the current ESLint behavior to apply and not to exempt React.

I'm sorry, I wish there was an easy way to do this, but this is precisely why we allow fine-grained control of rules using those comments.

I'd still encourage you to reach out to the React team to see if anything can be done.

In the meantime, if anyone has any other ideas, please feel free to share.

[gaearon]

Interesting, your response actually makes me think that using a comment to opt out is the best way forward.

It seems so! One minor thing I don't like about it is linter won't warn me if I'm using JSX and not importing React—JSXHint did that for me. Still, I can live with that :-)

Thank you for the discussion. Hopefully React team will solve this in the future.

[btmills]

One minor thing I don't like about it is linter won't warn me if I'm using JSX and not importing React

This is a perfect use case for a plugin - perhaps suggest this for yannickcr/eslint-plugin-react?

[sebmarkbage]

Future versions will not require React to be in scope to use JSX. That is one consideration.

On Feb 28, 2015, at 4:22 PM, Brandon Mills notifications@github.com wrote:

One minor thing I don't like about it is linter won't warn me if I'm using JSX and not importing React

This is a perfect use case for a plugin - perhaps suggest this for yannickcr/eslint-plugin-react?

—
Reply to this email directly or view it on GitHub.

[nzakas]

@sebmarkbage Thanks for the info, that totally makes sense based on this discussion.

Since there's not anything left to do here, I'm closing the issue. Thanks for the conversation everyone.

[glenjamin]

I was able to achieve the two goals using the existing rules extension behaviours, and have submitted them as PRs to https://github.com/yannickcr/eslint-plugin-react

1. Ensure that React is in scope whenever JSX is used
   add react-in-jsx-scope rule jsx-eslint/eslint-plugin-react#5
   This one is fairly straightforward: for every opening JSX tag, use the same algorithm as no-undef to make sure React is available.
2. Count JSX towards use of the React variable
   Add jsx-uses-react rule jsx-eslint/eslint-plugin-react#6
   This one is a bit trickier because it needs to silence some false-positives from no-unused-vars. The current approach I took was to take advantage of the internal implementation of the unused vars check, and have this new rule annotate the appropriate React variable whenever a JSX tag is opened. The tradeoffs of this approach are discussed in the linked PR.

These two checks should achieve the desired effect of changes suggested in this issue.