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

globalScope.__defineGeneric is not a function #49

Closed
e1cb4ac37s opened this issue Jan 25, 2024 · 12 comments · Fixed by #50 or #51
Closed

globalScope.__defineGeneric is not a function #49

e1cb4ac37s opened this issue Jan 25, 2024 · 12 comments · Fixed by #50 or #51

Comments

@e1cb4ac37s
Copy link

e1cb4ac37s commented Jan 25, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@e1cb4ac37s
Copy link
Author

e1cb4ac37s commented Jan 29, 2024

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
Copy link
Member

:/ 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?

@Brianzchen Brianzchen reopened this Jan 29, 2024
@e1cb4ac37s
Copy link
Author

e1cb4ac37s commented Jan 30, 2024

  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
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor

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

@Brianzchen
Copy link
Member

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
Copy link
Author

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

@short-dsb
Copy link
Contributor

Thanks, @Brianzchen! Really appreciate it. 🙂

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