flow-typed / eslint-plugin-ft-flow

https://www.npmjs.com/package/eslint-plugin-ft-flow
Other
19 stars 11 forks source link

`globalScope.__defineGeneric` is not a function #49

Closed e1cb4ac37s closed 8 months ago

e1cb4ac37s commented 8 months ago

globalScope.__defineGeneric is not a funtion

Hello. What problem do I try to solve: we try to migrate our out-of-date flow to the latest version, and on the way installed ft-flow to lint it. We have no-undef rule, and need to override it for flow internal types, i.e. $Values<...>, $Keys<...>, ..., because they conflict with eslint's no-undef now. I'm trying to add ft-flow to .eslintrs.js, which kinda works until I start adding rules beyond recommended extension. My config (relevant parts, I guess):

module.exports = {
  parser: 'hermes-eslint',
  parserOptions: {
    ecmaVersion: 9,
    sourceType: 'module',
    ecmaFeatures: {
      jsx: true,
    },
    babelOptions: {
      configFile: path.resolve(__dirname, './babel.config.js'),
    },
  },
  extends: [
    // ...
    'plugin:ft-flow/recommended'
  ],
  plugins: [
    'ft-flow',
    // ...
  ],
  env: {
    browser: true,
    jquery: true,
    node: true,
    jest: true,
    es6: true,
  },
  root: true,
  rules: {
    'ft-flow/use-flow-type': 1,
    'ft-flow/define-flow-type': 1, // this rule breaks
    // ...
  },
  // ...
}

The line of code where I get the error (I guess, $Values<...> breaks it)

export type ResourceType = $Values<typeof RESOURCE_TYPES>
ESLint: 8.56.0

TypeError: globalScope.__defineGeneric is not a function
Occurred while linting [some js file here]
Rule: "ft-flow/define-flow-type"
    at makeDefined (PATH/node_modules/eslint-plugin-ft-flow/dist/rules/defineFlowType.js:28:21)
    at GenericTypeAnnotation (PATH/node_modules/eslint-plugin-ft-flow/dist/rules/defineFlowType.js:67:9)
    at PATH/node_modules/eslint/lib/linter/timing.js:141:28
    at ruleErrorHandler (PATH/node_modules/eslint/lib/linter/linter.js:1076:28)
    at PATH/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (PATH/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (PATH/node_modules/eslint/lib/linter/node-event-generator.js:297:26)
    at NodeEventGenerator.applySelectors (PATH/node_modules/eslint/lib/linter/node-event-generator.js:326:22)
    at NodeEventGenerator.enterNode (PATH/node_modules/eslint/lib/linter/node-event-generator.js:340:14)

There is similar issue in your legacy (right?) project, but no solutions provided whatsoever. What can I do? Or maybe this is a bug? Thank you in advance.

Brianzchen commented 8 months ago

Thanks for raising this issue. At first glance it seems to be because __defineGeneric object does not exist in the hermes-eslint parser so I expect it should work with @babel/eslint-parser and as a result it's throwing.

But because with flow you should use hermes-eslint to parse the syntax correctly we'll have to find a solution for you. I'll keep digging to find what the equivalent with hermes would be.

Do note that flow team recommended that this rule isn't necessary anymore hence why we didn't test for this case

Brianzchen commented 8 months ago

Have raised an issue with hermes team. Lets see if they come back with anything useful before I cont my investigation. Otherwise I'll need to understand what that function did in the beginning to find a suitable replacement function

Brianzchen commented 8 months ago

^ Merged a fix. Please give a go with v3.0.3 and let me know if it works 🙏

e1cb4ac37s commented 8 months ago

Thank you for a very fast response and fix! ft-flow now does not throw, although eslint's no-unused-vars broke (which was not the case before). But it seems I have to go with this issue to eslint repo now - ft-flow's part works now. Thank you again.

Oops! Something went wrong! :(

ESLint: 8.56.0

TypeError: Cannot read properties of null (reading 'scope')
Occurred while linting PATH/client/src/core/api/some-js-file.js
Rule: "no-unused-vars"
    at getRhsNode (PATH/client/node_modules/eslint/lib/rules/no-unused-vars.js:356:43)
    at PATH/client/node_modules/eslint/lib/rules/no-unused-vars.js:541:27
    at Array.some (<anonymous>)
    at isUsedVariable (PATH/client/node_modules/eslint/lib/rules/no-unused-vars.js:534:40)
    at collectUnusedVariables (PATH/client/node_modules/eslint/lib/rules/no-unused-vars.js:658:26)
    at Program:exit (PATH/client/node_modules/eslint/lib/rules/no-unused-vars.js:677:36)
    at ruleErrorHandler (PATH/client/node_modules/eslint/lib/linter/linter.js:1076:28)
    at PATH/client/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (PATH/client/node_modules/eslint/lib/linter/safe-emitter.js:45:38)

Edit: in fact, I ran eslint without any recommendations (except ft-flow's one with 3 rules:

module.exports = {
  parser: 'hermes-eslint',
  parserOptions: {
    ecmaVersion: 9,
    sourceType: 'module',
    ecmaFeatures: {
      jsx: true,
    },
    babelOptions: {
      configFile: path.resolve(__dirname, './babel.config.js'),
    },
  },
  extends: ['plugin:ft-flow/recommended'],
  plugins: [
    'ft-flow',
  ],
  env: {
    browser: true,
    jquery: true,
    node: true,
    jest: true,
    es6: true,
  },
  root: true,
  globals: {
    globalThis: 'readonly',
  },
  rules: {
    'ft-flow/define-flow-type': 1, // (1)
    'ft-flow/use-flow-type': 1, // (2)
    'no-unused-vars': ['error', { argsIgnorePattern: '^_', ignoreRestSiblings: true }],
  },
  settings: {
    'import/resolver': {
      node: {
        paths: ['.'],
        extensions: ['.js', '.ts', '.jsx', '.tsx'],
      },
      'babel-module': {
        cwd: 'babelrc',
        root: ['./src/'],
        alias: babelAliases,
      },
    },
    'import/external-module-folders': [path.resolve(__dirname, './node_modules/')],
    'import/internal-regex': '^@+foxford',
    react: {
      version: '17',
    },
    flowtype: {
      onlyFilesWithFlowAnnotation: true,
    },
  },
  ignorePatterns: ['.eslintrc.js', '/docs', '/build', '/flow-typed', '**/node_modules/**', '*/*.sass'],
}

commenting out lines (1) and (2) makes `no-unused-vars' work, while keeping them leads to an error above

Brianzchen commented 8 months ago

:/ hmm I'll need to reproduce it in tests to find a solution. The change I made was to define-flow-type only and I suspect that when I inject the global variable it's missing something causing the next rule to fail.

    'ft-flow/define-flow-type': 1, // (1)
    'ft-flow/use-flow-type': 1, // (2)

For some more info

  1. Could you share the line or token that fails to be parsed causing this error?
  2. Is it when both rules are in use or only one?
  3. What happens if you only one of define-flow-type or use-flow-type?
e1cb4ac37s commented 8 months ago
  1. Here it is:

    // @flow
    
    import { Api } from 'services/api' // <- this one

    this looks like it breaks on a first line of the first flow-tagged file, as deleting this line moves an error to the next non-empty one, deleting the file moves error to the next file, first non-empty line.

  2. Indeed, commenting them out one-by-one shows that only define-flow-type breaks no-unused-vars, use-flow-type is ok.
Brianzchen commented 8 months ago

Great I was able to reproduce this, turns out we never had a test against no-unused-vars and only tested no-undef and no-use-before-define.

Brianzchen commented 8 months ago

@e1cb4ac37s can you try 3.0.4-alpha-0. I removed something that seemed unnecessary and was causing issues. It's throwing the unused errors correctly but need to spend more time getting the tests to play nicely together before I can merge and make an official release

short-dsb commented 8 months ago

FWIW 3.0.4-alpha-0 resolves the issue for me.

Brianzchen commented 8 months ago

Thanks for validating @short-dsb. I'll work on getting my tests working and publish an official release this weekend... Unless someone finds more problems 😨

e1cb4ac37s commented 8 months ago

Works for me now. Awesome job, Brian! Thank you!

short-dsb commented 8 months ago

Thanks, @Brianzchen! Really appreciate it. 🙂