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

False positive of no-import-module-exports on module variable defined in the module #2181

Open
zloirock opened this issue Aug 9, 2021 · 18 comments

Comments

@zloirock
Copy link
Contributor

zloirock commented Aug 9, 2021

import 'something'; // => Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')  import/no-import-module-exports

export const module = {};
module.foo = 42;
@ljharb
Copy link
Member

ljharb commented Aug 9, 2021

Is the variable “module” allowed in strict mode? If so, this is definitely a bug.

@zloirock
Copy link
Contributor Author

zloirock commented Aug 9, 2021

Sure, it's allowed. Otherwise, CommonJS would not work in strict mode -)

@ljharb
Copy link
Member

ljharb commented Aug 10, 2021

oh right, duh. thanks.

@micalevisk
Copy link

I'm facing this in eslint-plugin-import@2.25.3 after upgrading eslint-config-airbnb-base to 15.0.0

@ljharb
Copy link
Member

ljharb commented Nov 10, 2021

Per #2297, this should be fixed in v2.25.3.

@micalevisk can you share the code that’s erroring?

@micalevisk
Copy link

This is a TypeScript project that outputs Commonjs, btw.

every line of this:
image
image

and this:

image
image

.eslintrc.js
module.exports = {
  root: true,

  reportUnusedDisableDirectives: true,

  parserOptions: {
    ecmaVersion: 2020,
  },

  env: {
    es6: true,
    node: true,
    jest: true,
  },

  globals: {},

  extends: ['plugin:prettier/recommended'],
  plugins: ['import-helpers'],
  // Global rules
  rules: {
    'no-console': 'off',
    'no-bitwise': 'off',
    'no-plusplus': ['error', { allowForLoopAfterthoughts: true }],

    'max-len': 'off',

    'import-helpers/order-imports': [
      'error',
      {
        newlinesBetween: 'always',
        groups: [['module', '/^@[^/]/'], '/^@//', ['parent', 'sibling', 'index']],
        alphabetize: {
          order: 'asc',
          ignoreCase: true,
        },
      },
    ],
    'import/no-extraneous-dependencies': [
      'error',
      {
        packageDir: '.',
        // Files with scripts/modules that will be use only in development
        devDependencies: ['test/*', 'jest*', 'configs/**/*', 'tools/**/*'],
      },
    ],
  },

  overrides: [
    /* ==================== Dealing with TypeScript files ==================== */
    {
      files: ['*.ts'],
      parser: '@typescript-eslint/parser',
      parserOptions: {
        project: './tsconfig.lint.json',
        sourceType: 'module',
        ecmaVersion: 2020,
      },

      settings: {
        'import/parsers': {
          '@typescript-eslint/parser': ['.ts'],
        },
        'import/resolver': {
          typescript: {
            project: './tsconfig.lint.json',
          },
        },
      },

      plugins: ['jest', '@typescript-eslint', 'import'],
      extends: [
        'airbnb-base',
        'plugin:@typescript-eslint/recommended',
        'plugin:import/typescript',
        'plugin:@typescript-eslint/recommended-requiring-type-checking',
        'plugin:prettier/recommended',
        'plugin:jest/recommended',
      ],
      rules: {
        'jest/expect-expect': [
          'error',
          {
            assertFunctionNames: ['expect', 'request.**.expect'],
          },
        ],

        'no-shadow': 'off', // Disable this one in favor of the TS-version one
        '@typescript-eslint/no-shadow': 'error',

        'default-param-last': 'off', // Disable this one in favor of the TS-version one
        '@typescript-eslint/default-param-last': 'error',

        '@typescript-eslint/no-unused-vars': [
          'warn',
          { ignoreRestSiblings: true, argsIgnorePattern: '^_' },
        ],
        '@typescript-eslint/no-useless-constructor': 'error',
        '@typescript-eslint/no-explicit-any': 'off',
        // '@typescript-eslint/no-unsafe-return': 'off',
        '@typescript-eslint/no-non-null-assertion': 'off',
        '@typescript-eslint/explicit-module-boundary-types': [
          'error',
          {
            allowArgumentsExplicitlyTypedAsAny: true,
            allowDirectConstAssertionInArrowFunctions: true,
            allowedNames: [],
            allowHigherOrderFunctions: true,
            allowTypedFunctionExpressions: true,
          },
        ],

        'import/order': 'off',
        'import/prefer-default-export': 'off',
        'import/no-unresolved': 'off', // Let TS handle this
        'import/named': 'off', // and this, due to some resolution error
        'import/extensions': [
          'error',
          {
            json: 'always',
            ts: 'never',
            tsx: 'never',
            js: 'never',
            jsx: 'never',
          },
        ],
        'import/no-extraneous-dependencies': [
          'off', // Let typescript compiler complains about this.
          {
            // In case we want to enable this rule.
            packageDir: '.',
            // Files with scripts/modules that will be use only in development
            devDependencies: ['**/*spec.ts', '**/global.d.ts', '**/*.seed.ts', '**/*.factory.ts'],
          },
        ],

        /* ==================== overrides Airbnb's rules ==================== */
        'class-methods-use-this': 'off',
        'max-classes-per-file': ['error', 2], // Max 2 classes per file
        'max-len': [
          'error',
          {
            code: 100, // same as `printWidth` from Prettier
            comments: 83,
            ignoreTrailingComments: true,
            ignoreUrls: true,
            ignoreStrings: true,
            ignoreTemplateLiterals: true,
            ignoreRegExpLiterals: true,
          },
        ],
        'no-bitwise': 'off',
        'no-useless-constructor': 'off',
        'no-use-before-define': 'off',
        'no-unused-vars': 'off',
        'no-underscore-dangle': 'off',
        // Overrides https://github.com/airbnb/javascript/blob/b6fc6dc7c3cb76497db0bb81edaa54d8f3427796/packages/eslint-config-airbnb-base/rules/style.js#L257
        // See https://github.com/airbnb/javascript/issues/1271
        'no-restricted-syntax': ['error', 'ForInStatement', 'LabeledStatement', 'WithStatement'],
      },
    },

    /* ======================= Dealing with edge cases ======================= */
    {
      // Scripts-like files under `src` directory.
      files: ['src/scripts/**/*'],
      rules: {
        'no-console': 'off',
      },
    },
    {
      // Entities/models typescript files under `src` directory.
      files: ['src/**/*.entity.ts'],
      rules: {
        // Allow define relations between database tables, while using JS classes.
        // NOTE: This leave rooms to `undefined` values when using `import` in these
        //       modules.
        'import/no-cycle': 'off',
      },
    },

    /* ======================= Dealing with Jest files ======================= */
    {
      files: ['**/*spec.ts'],
      rules: {
        'max-classes-per-file': 'off',
        'no-multi-assign': 'off',

        'import/no-extraneous-dependencies': 'off',

        '@typescript-eslint/no-empty-function': 'off',
        '@typescript-eslint/explicit-module-boundary-types': 'warn',
        '@typescript-eslint/unbound-method': 'off',
      },
    },
  ],
};

$  grep version node_modules/eslint-plugin-import/package.json 
  "version": "2.25.3",

$  grep version node_modules/eslint-config-airbnb-base/package.json 
  "version": "15.0.0",

@ljharb

This comment has been minimized.

@micalevisk

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Nov 10, 2021

@micalevisk if you could set up a repro repo that would be most helpful.

@micalevisk
Copy link

I figured out that I'm getting those lint errors when I access any property of module variable that was created by webpack.

image

image

Here's the code: https://gitlab.com/micalevisk/eslint-plugin-import-issue-2181

I might be missing something. I appreciate your help @ljharb

@ljharb
Copy link
Member

ljharb commented Nov 13, 2021

module is something that's only supposed to exist in CJS - if webpack is creating it in ESM, that's going to break lots of static analysis.

That said, we could refine the check so that accessing statically knowable property names of module that are not "exports" does not trigger the rule.

@micalevisk
Copy link

this is a TS project that targets CJS, not ESM. Maybe I need to configure some plugin then?

@ljharb
Copy link
Member

ljharb commented Nov 14, 2021

What's targeted shouldn't matter, only the source; module isn't available in ESM, and your source is ESM.

My guess is that there's a way webpack can use import.meta instead of module, and you'd both have less confusing code, and also bypass this overly-aggressive lint rule (that said, i'm still happy to remove some false positives)

@micalevisk
Copy link

right

unfortunately I can't use import because the module system I'm using is CJS (due to "module": "commonjs" entry on my tsconfig)

The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'esnext', or 'system'.ts(1343)

@snitin315
Copy link

@ljharb I can reproduce it with the following code as per eslint-plugin-import@2.26.0:

import React from 'react'; // error   Cannot use import declarations in modules that export using ...
import { render } from 'react-dom'; // error
import { loadableReady } from '@loadable/component'; // error
import { BrowserRouter } from 'react-router-dom'; // error
import { HelmetProvider } from 'react-helmet-async'; // error


if (module.hot) {
  render(app, root);
  module.hot.accept();
} else {
  loadableReady(() => {
    hydrateApp(app, root);
  });
}

If I remove the module.hot condition there is no error, the following code is lint free.

import React from 'react';
import { render } from 'react-dom'; 
import { loadableReady } from '@loadable/component'; 
import { BrowserRouter } from 'react-router-dom';
import { HelmetProvider } from 'react-helmet-async';

@ljharb
Copy link
Member

ljharb commented Dec 13, 2022

@snitin315 i definitely can reproduce the warnings, but they seem valid to me - module isn't in scope in ES Modules. I'm not sure how to resolve this.

@sangaline
Copy link

@snitin315 The solution for the module.hot issue with webpack's [HotModuleReplacementPlugin](https://webpack.js.org/plugins/hot-module-replacement-plugin) is to use import.meta.webpackHot.

@ljharb ljharb closed this as completed Sep 30, 2023
@ljharb
Copy link
Member

ljharb commented Sep 30, 2023

actually reopening for the false positives; but “webpack” isn’t a problem per the above.

@ljharb ljharb reopened this Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants