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

no-unresolved JS alias regression in 3.8+ #363

Open
RobHannay opened this issue Feb 20, 2025 · 29 comments
Open

no-unresolved JS alias regression in 3.8+ #363

RobHannay opened this issue Feb 20, 2025 · 29 comments

Comments

@RobHannay
Copy link

Since 3.8 aliases are no-longer being resolved in .js files.

I've read a few of the issues raised about 3.8, but I think this is separate. I have tested with 3.8.3 and still face the issue.

We use aliases that are actually defined in three possible places:

  • webpack.config
  • jsconfig
  • tsconfig

Image

Perhaps it was never intended to resolve imports from js files?

It's fine is I use relative paths with lots of ../.

We have a verbose config but the key details are:

{
  eslint.configs.recommended,
  ...tseslint.configs.recommendedTypeChecked,
  importPlugin.flatConfigs.recommended,
  {
    languageOptions: {
      parserOptions: {
        projectService: true,
        tsconfigRootDir: import.meta.dirname,
      },
    },
  },
  {
    files: ['**/*.js', '**/*.mjs'],
    ...tseslint.configs.disableTypeChecked,
  },
  {
    languageOptions: {
      globals: {
        ...globals.node,
        ...globals.jest,
      },
    },
    settings: {
      'import/parsers': {
        '@typescript-eslint/parser': ['.ts', '.tsx'],
        '@babel/eslint-parser': ['.js', '.mjs'],
      },
      'import/resolver': {
        typescript: {
          alwaysTryTypes: true,
        },
      },
      // Gives false positives because RRD has two versions in our dependency tree.
      'import/ignore': ['react-router-dom'],

      'import/internal-regex': aliasesRegex,
    },
    rules: {
      'import/order': [
        'error',
        {
          'newlines-between': 'always',
          groups: ['type', 'external', 'internal', ['parent', 'sibling', 'index'], 'object'],

          alphabetize: {
            order: 'asc',
          },
        },
      ],

      'import/no-cycle': 'error',

      'import/no-unresolved': [
        'error',
        {
          caseSensitiveStrict: true,
        },
      ],

      'workspaces/no-cross-imports': [
        'error',
        {
          allow: '@blink/core',
        },
      ],

      'no-restricted-imports': [
        'error',
        {
          patterns: [
            {
              group: ['*.css', '!*.module.css'],
              importNames: ['default'],
              message: 'For CSS modules, use the `.module.css` extension.',
            },
          ],
        },
      ],

    },
  },
}

Happy to try and create a proper repro repo. Do you have a base I can clone?

@carlocorradini
Copy link
Contributor

The aliases are resolved only if the file is included from the corresponding tsconfig.json or jsconfig.json.
If you specificy an alias (i.e. A) in tsconfig.json, and file X has the alias but it's not present in include or files of tsconfig.json then it's not resolved.
How do you include the corresponding file in your configuration?

@RobHannay
Copy link
Author

RobHannay commented Feb 20, 2025

tsconfig just has a blanket

   "include": ["**/*.ts", "**/*.tsx"],

jsconfig has none of the above.

I can't work out what specifically in 3.8 has changed this behaviour

I've also tried adding

  "include": ["**/*.js"],

to my jsconfig but it's not removed the error.

I know it's possible this could have been an unintentional feature, supporting alias resolution in JS files, but should it revert to Webpack resolution throughout ts/js files as that's what's really being done during builds?

Is there a way that I can debug the resolved alias paths?

@carlocorradini
Copy link
Contributor

Can you create a repro for a better context (Thanks)

@mkosir
Copy link

mkosir commented Feb 20, 2025

Not sure if the same issue 🤔 but I’m experiencing similar in project react-parallax-tilt.
You can reproduce it by upgrading the dependency version to latest 3.8.3 and running npm run lint

@carlocorradini
Copy link
Contributor

Mhmhm I see a lot of ../ in both include and exclude.
Tomorrow I'll try to find the cause.
@SuperchupuDev do you think we have something similar to the other issue?

@SuperchupuDev
Copy link

SuperchupuDev commented Feb 20, 2025

oh! @mkosir just to confirm, if you do <your package manager of choice> why tinyglobby it shows up as 0.2.12, right?

@carlocorradini probably! it's definitely related to the same function :P but i need a minimal repro (as in glob calls, not an entire react project) to investigate regardless

@estiller
Copy link

estiller commented Feb 20, 2025

HI, I'm using NX as a monorepo tool, and I see the exact behaviour as well. NX relies on aliases in tsconfig to manage inter-package dependencies, and when I upgrade to eslint-import-resolver-typescript version 3.8.0 or above (including 3.8.3) I get the exact same error as OP. It works fine up to version 3.7.0.

I ran pnpm why tinyglobby, and it indeed resolved to 0.2.12. I tried overriding it to use 0.2.10, but I got the same result.

@carlocorradini
Copy link
Contributor

@estiller Can you provide a repro?
Thanks

@RobHannay
Copy link
Author

Here's a repro https://github.com/RobHannay/typescript-import-resolver-repro

Version pinned to 3.7.0 which works fine. Bumping up to 3.8.3 causes the lint error in app/app.js

Image

@pralkarz
Copy link

pralkarz commented Feb 20, 2025

Not sure if the same issue 🤔 but I’m experiencing similar in project react-parallax-tilt. You can reproduce it by upgrading the dependency version to latest 3.8.3 and running npm run lint

I'm not sure about the other issues listed here, but I investigated this one in particular, and it seems like it's potentially a problem with get-tsconfig. After https://github.com/mkosir/react-parallax-tilt/blob/main/tsconfig.json is resolved by extending https://github.com/mkosir/react-parallax-tilt/blob/main/scripts/tsconfig.dev.json, "include": ["../.", "../.storybook/**/*", "../.commitlintrc.ts"] gets parsed as [ '', '.storybook/**/*', '.commitlintrc.ts' ] and passed to tinyglobby there:

? globSync(tsconfigResult.config.include, {

The empty string doesn't match anything and the no-unresolved errors are raised. When manually patching that string to be '.' or './**/*', everything works fine.

A workaround could be to change ../. to .././**/* (in which case get-tsconfig parses it as **/*), but a more robust fix should probably happen in get-tsconfig. I'm not sure why it parses ../. into an empty string instead of ..

@carlocorradini
Copy link
Contributor

@pralkarz Wow thanks for your discovery/help.
Tomorrow I'll try the other repro

@RobHannay
Copy link
Author

Here's a repro RobHannay/typescript-import-resolver-repro

Version pinned to 3.7.0 which works fine. Bumping up to 3.8.3 causes the lint error in app/app.js
Image

Just went through a bunch of stuff to clean out some definitely-unrelated bits of the config and have pushed that up now too.

Here's the evidence of that repro:

Image

@pralkarz
Copy link

pralkarz commented Feb 20, 2025

Here's a repro https://github.com/RobHannay/typescript-import-resolver-repro

Version pinned to 3.7.0 which works fine. Bumping up to 3.8.3 causes the lint error in app/app.js
Image

I took a look at this one too. Seems like mapping aliases is handled differently in 3.8.x vs 3.7.0. In

mappers = projectPaths
the mappers are created, but aren't used in any way while globbing, and only returned at the end. I'm not very familiar with this logic, so I can't gauge what goes wrong there, but it seems like it's unrelated to tinyglobby. 🤔

EDIT after some more digging (disregard the above guess): Adding '**/*.js' to tsconfig.json fixes the issue. Without it, app.js file isn't matched, and so the .has check at

.filter(({ files }) => files.has(file))
never returns true. I've compared that to how it was handled in 3.7.0, and removing the filter seems to fix the issue as well, although it makes the linting a little bit longer.

CC: @carlocorradini @RobHannay @SuperchupuDev

@carlocorradini
Copy link
Contributor

@pralkarz @RobHannay
This is the correct behavior.
If app.js (or any other file) is not included in your tsconfig.json, how does that file determine the correct path mapping?
Version prior to 3.8 check if that path is known even if it is not found in any of the known tsconfig.json (or jsconfig.json) files. This raises multiple issues. One example is where the same mapping alias appears in various tsconfig.json files, but the destination (file referred to) differs. The latter is widely used in monorepo, where each package might have the same mapping alias (i.e. @), but the destination corresponds to the src directory of that package.
See issue #246

@RobHannay
Copy link
Author

RobHannay commented Feb 21, 2025

Thanks @carlocorradini, that makes sense! As I suspected then, in that we were actually relying on a bug.

I know bug fixes aren't usually considered "breaking" in the traditional sense, but I wonder if this (correct) behavioural change might cause breakages for a number of people that were relying on it.

Are you saying that adding include to my jsconfig should also match the paths in there to resolve this issue for me?

@JounQin
Copy link
Collaborator

JounQin commented Feb 21, 2025

@RobHannay Does VSCode/tsc itself complains no module resolved with your jsconfig/tsconfig settings? Yes so I think that's a bug fix for unintended behavior, but if not, we may need find out how does tsc fallback aliasing?

cc @carlocorradini

@RobHannay
Copy link
Author

Both VS Code and WebStorm happily resolve the import paths.

Screen.Recording.2025-02-21.at.10.33.02.mov

@JounQin
Copy link
Collaborator

JounQin commented Feb 21, 2025

So when no matched project found it still should fallback to its nearest listed project, how do you think? @carlocorradini

@RobHannay
Copy link
Author

RobHannay commented Feb 21, 2025

Another small thing to note that I've just been looking at.

How do you include the corresponding file in your configuration?

As I mentioned, in our jsconfig we actually only have an exclude property, without include. According to the docs, that is sufficient:

https://code.visualstudio.com/docs/languages/jsconfig#_using-the-exclude-property

Adding include doesn't fix it anyway at the moment, and if that's what's needed then I'd be happy to add it. But according to documentation it shouldn't be necessary

@carlocorradini
Copy link
Contributor

Another small thing to note that I've just been looking at.

How do you include the corresponding file in your configuration?

As I mentioned, in our jsconfig we actually only have an exclude property, without include. According to the docs, that is sufficient:

https://code.visualstudio.com/docs/languages/jsconfig#_using-the-exclude-property

Adding include doesn't fix it anyway at the moment, and if that's what's needed then I'd be happy to add it. But according to documentation it shouldn't be necessary

If no include and no files property are present we include everything ("**/*"). Same behavior as Typescript. Note that we apply your exclude if present

@carlocorradini
Copy link
Contributor

Both VS Code and WebStorm happily resolve the import paths.

Screen.Recording.2025-02-21.at.10.33.02.mov

The editor may be smart but if you compile with TSC the JS file Is not present in the outdir (can you try?)

@RobHannay
Copy link
Author

Both VS Code and WebStorm happily resolve the import paths.

Screen.Recording.2025-02-21.at.10.33.02.mov

The editor may be smart but if you compile with TSC the JS file Is not present in the outdir (can you try?)

It does get emitted by tsc

Image

@carlocorradini
Copy link
Contributor

@RobHannay The .js file is emitted in the dist folder even though it's not present in both include and files? How is it possible?!
Can anyone explain this? I'm confused 😕

@RobHannay
Copy link
Author

My guess would be that it's imported by a .ts(x) file at some point, so tsc needs to include it in the dist

@ehoogeveen-medweb
Copy link

ehoogeveen-medweb commented Feb 21, 2025

Might be worth running tsc with --explainFiles (or setting "explainFiles": true in the compiler options) to see what it says.

Edit: And maybe traceResolution for detailed info on how it resolves a particular file (I'd recommend dumping the output to a file for that).

@RobHannay
Copy link
Author

RobHannay commented Feb 21, 2025

Nice. Well I can see it's just imports as suspected:

(blah/blah)/components/Primer/Primer.js
  Imported via './components/Primer/Primer' from file 'webapp/src/components/Login/Login.js'

Edit with traceResolution, full stack back to an included file. Approximately:

path/to/components/Text/Text.js
  Imported via 'components/Text/Text' from file 'path/to.js'
  Imported via '../Text/Text' from file 'path/to.js'
  Imported via 'components/something' from file 'path/to/some.tsx'

@JounQin
Copy link
Collaborator

JounQin commented Feb 21, 2025

So when no matched project found it still should fallback to its nearest listed project, how do you think? @carlocorradini

@carlocorradini Fallback to nearest tsconfig.json is desired behavior IMO.

@carlocorradini
Copy link
Contributor

We have two different types of issues:

  1. Thanks to @pralkarz, we discovered that if include contains ../. (or .), get-tsconfig returns an empty string ("").
    I've opened an issue: Include . resolves to an empty string privatenumber/get-tsconfig#88
    @mkosir The change from ../. to .././**/* works. You can modify it or wait till the get-tsconfig issue is fixed.
  2. @RobHannay To fix this, we need to modify from line
    .filter(({ files }) => files.has(file))

    Pseudocode:
    If mappedPath (resolved file path) already exists in the associated Set: do nothing.
    Otherwise, we add this new file (mappedPath) to the associated Set to ensure that future resolutions have the file matched with the corresponding (correct) mapping function.

What are your thoughts? 🫡

I'll try to fix this in a few days 😋

@carlocorradini
Copy link
Contributor

@JounQin See https://github.com/carlocorradini/eslint-import-resolver-typescript/tree/363
mapping is a Map where the key is the absolute (native) path of a file and the value is a Set of paths matching functions (applicable).
When the getMappedPath function is called, we ensure that each mappedPath exists in the mapping: If not, add the new path with the corresponding functions; otherwise, add any (possible) missing functions.
Everything works however there is catch:
Each file that has to be linted is (mostly) evaluated at random (try this with yarn run test:importTransitive). As a result, a file that is transitively imported from another and does not yet exist in the mapping may be evaluated first, returning not found. However, subsequently, following the import tree (in this example from index.ts), we properly populate mapping with the correct file paths/functions, and all mapping are fully resolved: You can see this in the log.
This is because import/import-x calls resolve once, therefore correct path resolution (later) from the same file is discarded/forgotten.
To solve this, if the file is not in the mapping, we attempt to resolve the path mapping using all known functions. However, this can produce numerous different possible paths for the same alias, causing the same as issue #246. Or we can force to list all files in the tsconfig.json.
Which one do you prefer?

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

8 participants