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

Typescript + React + Context.Provider = TypeError: Cannot read property 'variables' of null #2882

Closed
stevematney opened this issue Dec 22, 2020 · 18 comments

Comments

@stevematney
Copy link

stevematney commented Dec 22, 2020

eslint is failing when using @typescript-eslint/eslint-plugin and eslint-react-plugin together. This failure seems to only happen when attempting to render a Provider from a Context as jsx.

If I disable react/jsx-no-undef, it works, and I can fall back to tsc or tsserver to catch JSX undefined errors, but then eslint itself becomes silent on the issue. Here's the configuration I've got in a stripped down project to produce the error. All the relevant files are listed below, and here is a repo which demonstrates the issue: https://github.com/stevematney/eslint-clash

tsconfig.json

{
  "compilerOptions": {
    "target": "ES2019",
    "lib": ["dom", "dom.iterable", "ES2019"],
    "allowJs": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "module": "commonjs",
    "moduleResolution": "Node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "downlevelIteration": true,
    "jsx": "react"
  },
  "include": ["*", "./.eslintrc.js"]
}

.eslintrc.js

module.exports = {
  root: true,
  env: {
    browser: true,
    node: true,
  },
  extends: [
    "plugin:react/recommended",
    "plugin:@typescript-eslint/recommended",
  ],
  parser: "@typescript-eslint/parser",
  parserOptions: {
    sourceType: "module",
    ecmaFeatures: {
      jsx: true,
      modules: true,
    },
    ecmaVersion: "es2020",
    tsconfigRootDir: __dirname,
    project: `./tsconfig.json`,
  },
  settings: {
    react: {
      version: "detect",
    },
  },
};

App.tsx

import React, { createContext } from "react";

const basicContext = { yes: "yes", no: "no" };
const AContext = createContext(basicContext);

export default (function () {
  return (
    <AContext.Provider value={basicContext}>
      {/* put stuff in here if you want */}
    </AContext.Provider>
  );
} as React.FC);

package.json

{
  "name": "eslint-clash",
  "version": "1.0.0",
  "description": "",
  "main": ".eslintrc.js",
  "author": "",
  "license": "ISC",
  "dependencies": {
    "@typescript-eslint/eslint-plugin": "^4.11.0",
    "@typescript-eslint/parser": "^4.11.0",
    "eslint": "^7.16.0",
    "eslint-plugin-react": "^7.21.5",
    "react": "^16.14.0"
  }
}

Error Output:

TypeError: Cannot read property 'variables' of null
Occurred while linting /Users/me/MacsideProjects/eslint-clash/hello.tsx:8
    at checkIdentifierInJSX (/Users/me/node_modules/eslint-plugin-react/lib/rules/jsx-no-undef.js:63:27)
    at JSXOpeningElement (/Users/me/node_modules/eslint-plugin-react/lib/rules/jsx-no-undef.js:106:9)
    at /Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
    at /Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/linter.js:952:32

I would be happy to hear if I'm just doing something incorrectly, but this does seem to be a bug.

Thanks!

@ljharb
Copy link
Member

ljharb commented Dec 22, 2020

What is the failure? You haven't provided it yet.

@ljharb
Copy link
Member

ljharb commented Dec 22, 2020

Also, what version of react are you using? React 17 uses a different jsx transform. With the "legacy" jsx transform, you have to enable the react/jsx-uses-react rule (which the recommended config should do) so that the existence of jsx causes React.createElement to be marked as "used".

@stevematney
Copy link
Author

stevematney commented Dec 22, 2020

What is the failure? You haven't provided it yet.

You're right, @ljharb! I updated the description with the package.json and the error output. Here is the error output again:

TypeError: Cannot read property 'variables' of null
Occurred while linting /Users/me/MacsideProjects/eslint-clash/hello.tsx:8
    at checkIdentifierInJSX (/Users/me/node_modules/eslint-plugin-react/lib/rules/jsx-no-undef.js:63:27)
    at JSXOpeningElement (/Users/me/node_modules/eslint-plugin-react/lib/rules/jsx-no-undef.js:106:9)
    at /Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (/Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
    at /Users/me/MacsideProjects/eslint-clash/node_modules/eslint/lib/linter/linter.js:952:32

Also, what version of react are you using?

I was using React 17, so I switched to 16 and the error remains the same, unfortunately.

@ljharb
Copy link
Member

ljharb commented Dec 22, 2020

I'm unable to reproduce this - your test case passes with babel-eslint as well as with both the old and new TypeScript eslint parsers.

Can you provide the exact, unedited contents of hello.tsx? The OP refers to App.tsx.

@stevematney
Copy link
Author

@ljharb App.tsx in the example is exactly the same as hello.tsx locally. I've tried changing typescript versions and react versions, but I consistently get this error. I've created a repo that demonstrates it. I was able to remove the directory locally and re-clone it, and running eslint still produces the error.

Here's the demonstration repo: https://github.com/stevematney/eslint-clash

Thank you for your help!

@stevematney
Copy link
Author

Also, if it helps, I'm running node on v15.4.0.

@ljharb
Copy link
Member

ljharb commented Dec 22, 2020

Thanks, I'm able to reproduce it locally :-) once i get that turned into a test case, i should be able to land a fix.

@stevematney
Copy link
Author

That’s excellent! Thank you!

@ljharb
Copy link
Member

ljharb commented Dec 22, 2020

Hmm, this is very confusing. The error in your repo goes away when I comment out ecmaVersion, or when I comment out the "plugin:@typescript-eslint/recommended", line. I can't seem to reproduce it in this repo's tests, even when i temporarily use the same versions of typescript and the TS eslint parser.

The issue seems to be that somehow the "scope" object eslint is seeing doesn't have a type of "module" - iow, the scope jumps straight from function to global, without the intervening module that should be there.

I have a fix, but I'm hesitant to land it without a regression test :-/

@stevematney
Copy link
Author

stevematney commented Dec 22, 2020

I believe what we've discovered is actually either a bug or an unclarity in @typescript-eslint. Their docs say that ecmaVersion is a number, but the examples in the docs seem to suggest that you can use a string representing the actual version you're targeting, like "es2020".

This code is how they're parsing the version, and it looks like what's happening is it eventually evaluates the version I've listed as esNaN, which of course doesn't exist, so it defaults to es5.

I set the ecmaVersion to 5 and got this same error. If I set it to any post-es6 version (like 2020 or even just 6), it works fine. So ultimately I think this is a problem on their end and not in eslint-plugin-react, even though that's where it's ultimately failing.

@ljharb
Copy link
Member

ljharb commented Dec 23, 2020

aha! That makes sense. That might help me figure it out.

ecmaVersion is absolutely and always only a number, fwiw.

@ljharb
Copy link
Member

ljharb commented Dec 23, 2020

When I set ecmaVersion to 5, I get A fatal parsing error occurred: Parsing error: ImportDeclaration should appear when the mode is ES6 and in the module context. in my test cases.

@stevematney
Copy link
Author

It's interesting that you get a different error. If I just change ecmaVersion to 5, I still get the same error as reported above when I run it. I'm just running it directly through the cli, so there may be some difference between that and how your tests are executing eslint.

Do you think this still represents a bug in the plugin, since it's breaking on es5 in this scenario, or is this simply a scenario that should never happen if you're intentionally targeting es5?

@ljharb
Copy link
Member

ljharb commented Dec 23, 2020

I think it's a scenario that should never happen, and you might be right that RuleTester is stricter than the CLI is, out of necessity.

I'll land the fix anyways, since it doesn't hurt, but i do think that the typescript parser should be erroring out when the ecmaVersion isn't high enough - import syntax shouldn't work at all when ecmaVersion is set to 5 (cc @bradzacher)

@stevematney
Copy link
Author

That's true. It seems interesting that it "supports" es5 when TS is very heavily geared toward writing in post-es6 syntax. That said, I think it's theoretically possible to write very simple es5 code and include Type information, and it should still function fine.

Something like:

interface Vehicle {
  canDrive: boolean;
}

var myCar: Vehicle = { canDrive: true };

That's fully valid es5 code and fully valid TypeScript (I think), so there shouldn't be an issue attempting to lint it. I think I would hope to see an error earlier in the process when my ecmaVersion is set to 5 and the parser sees something that's expressly not compatible (like the error you're seeing in your tests) rather than an error much later when eslint-plugin-react tries to run a rule against it.

@bradzacher
Copy link
Contributor

bradzacher commented Dec 23, 2020

import syntax shouldn't work at all when ecmaVersion is set to 5

Our underlying parser doesn't respect the ecmaVersion parserOption.
Never has - and intentionally so.

The intention was never that the parser be used as a validation tool like it is for plain JS.
TS itself (external to your lint run) will provide syntactic and semantic validation (and more) and has better access to your project's actual build-time configs.

Additionally - ecmaVersion is an odd concept when it comes to typescript considering that it will (similar to babel) allow you to write any code and have it transpiled down to a target version.

Setting ecmaVersion to 5 for TS is pretty meaningless.
You cannot write valid TS code and ship it to a runtime because types are not valid JS code - it has to be transpiled first.

So for TS - writing import X from 'y' is (for all intents and purposes) valid ES5 syntax - because under the hood it will transpile it down to whatever module system you've configured it to target and ensure it's compliant with whatever ecmaVersion you've configured.


We have only recently added some handling for the ecmaVersion. That handling is only in the scope manager, and it is kind of just a backwards compatibility thing due to the fact that our scope manager is essentially a fork of eslint-scope.

@ljharb
Copy link
Member

ljharb commented Dec 23, 2020

@bradzacher that's a totally fair point - it seems like this bug is indeed caused by something in the scope manager. Perhaps your fork of eslint-scope could be made to ignore the parserOptions.ecmaVersion (since i'm assuming what's happening here is that it's not ignoring it)?

@bradzacher
Copy link
Contributor

bradzacher commented Dec 23, 2020

yeah so essentially what happened here is that the code is just super naive assumes ecmaVersion will be a number or nullish.

Because 'es2020' is non-nullish, it is passed through the scope analyser:
https://github.com/typescript-eslint/typescript-eslint/blob/535c8c438a3328c92611daa239343e2f610cbc83/packages/scope-manager/src/analyze.ts#L96

And then the "is ES6" check fails because 'es2020' is not >= 6!
https://github.com/typescript-eslint/typescript-eslint/blob/535c8c438a3328c92611daa239343e2f610cbc83/packages/scope-manager/src/ScopeManager.ts#L83

But yeah - it should 100% just ignore ecmaVersion - there's no reason for us to honour it in scope analysis as TS will never behave like as if it's ES5.

Filed typescript-eslint/typescript-eslint#2900

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

No branches or pull requests

3 participants