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

resolve.alias does not resolve correctly when been resolved as soft links in node_modules #4842

Closed
6 tasks done
JounQin opened this issue Jan 1, 2024 · 20 comments
Closed
6 tasks done

Comments

@JounQin
Copy link

JounQin commented Jan 1, 2024

Describe the bug

----------------------------------|---------|----------|---------|---------|-------------------

File % Stmts % Branch % Funcs % Lines Uncovered Line #s
All files 0.84 0 0 1.18
eslint-mdx/lib 0 0 0 0
index.es2015.mjs 0 0 0 0 5-196
eslint-mdx/src 0.73 0 0 1.17
helpers.ts 0 0 0 0 1-140
index.ts 100 100 100 100
meta.ts 0 100 100 0 1-3
parser.ts 0 0 0 0 1-100
sync.ts 0 100 100 0 1-11
tokens.ts 0 0 0 0 6-272
worker.ts 0 0 0 0 3-775
eslint-plugin-mdx/lib 0 0 0 0
index.es2015.mjs 0 0 0 0 7-508
eslint-plugin-mdx/src 33.33 0 0 45.45
helpers.ts 0 0 0 0 1-15
index.ts 100 100 100 100
meta.ts 0 100 100 0 1-3
eslint-plugin-mdx/src/configs 0 0 0 0
base.ts 0 100 100 0 3
code-blocks.ts 0 100 100 0 3
flat.ts 0 100 100 0 2-30
index.ts 0 100 0 0 3-11
overrides.ts 0 0 100 0 2-13
recommended.ts 0 0 0 0 1-95
eslint-plugin-mdx/src/processors 0 0 0 0
helpers.ts 0 0 0 0 1-23
index.ts 0 100 0 0 1-8
options.ts 0 0 0 0 9-55
remark.ts 0 0 0 0 2-67
eslint-plugin-mdx/src/rules 0 0 0 0
index.ts 0 100 0 0 2-8
remark.ts 0 0 0 0 1-115
---------------------------------- --------- ---------- --------- --------- -------------------

I'd expect no eslint-mdx/lib and eslint-plugin-mdx/lib files here.

Reproduction

https://github.com/mdx-js/eslint-mdx/actions/runs/7378227896/job/20073106362?pr=508#step:6:27

https://github.com/mdx-js/eslint-mdx/tree/build/yarn_v4

System Info

System:
    OS: macOS 14.2.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 11.77 GB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.10.0 - ~/Library/Caches/fnm_multishells/89034_1703864877615/bin/node
    Yarn: 4.0.2 - ~/Library/Caches/fnm_multishells/89034_1703864877615/bin/yarn
    npm: 10.2.3 - ~/Library/Caches/fnm_multishells/89034_1703864877615/bin/npm
    bun: 1.0.20 - /opt/homebrew/bin/bun
  Browsers:
    Brave Browser: 120.1.61.109
    Chrome: 120.0.6099.129
    Safari: 17.2.1
  npmPackages:
    @vitest/coverage-istanbul: ^1.1.1 => 1.1.1 
    vitest: ^1.1.1 => 1.1.1

Used Package Manager

yarn

Validations

@sheremet-va
Copy link
Member

I'd expect no eslint-mdx/lib and eslint-plugin-mdx/lib files here.

Why you don't expect them?

@JounQin
Copy link
Author

JounQin commented Jan 2, 2024

Because I have resolve.alias settings with eslint-mdx/eslint-plugin-mdx? eslint-mdx/src/index.ts should be resolved.

@sheremet-va
Copy link
Member

And how is node_modules involved here? This is hardly a minimal reproduction.

@JounQin
Copy link
Author

JounQin commented Jan 2, 2024

@sheremet-va Yes, this could be a complicated issue.

I'm using eslint-plugin-mdx@link:packages/eslint-plugin-mdx.

See also https://github.com/mdx-js/eslint-mdx/blob/cf077e5e0e6d85ccb4dbeca142cb6e30e67dde8d/package.json#L60

eslint will resolve mdx plugin automatically in the test cases. (Non-flat config mode)

@sheremet-va
Copy link
Member

sheremet-va commented Jan 2, 2024

resolve.alias is used only for:

  • static/dynamic imports using ESM syntax (await import or import * from)
  • the file that does the import is processed (inlined) by Vitest. Libraries by default are not inlined if Node.js can process them without Vite (file is correct CJS/ESM file). This is a performance optimization

So, try adding the library to server.deps.inline first.

@JounQin
Copy link
Author

JounQin commented Jan 4, 2024

image

@sheremet-va Seems not working neither.

@sheremet-va
Copy link
Member

sheremet-va commented Jan 4, 2024

Inlining your own package doesn't do anything - your source code is already inlined. The option is located inside deps name because it is meant to list dependencies that need inlining. On your screenshot, you also don't have any aliases.

@JounQin
Copy link
Author

JounQin commented Jan 4, 2024

So what should I do then? I can not inline eslint because I have to use setup file which includes a specific eslint subpath to preload it for testing. This approach works well with jest.

See also https://github.com/mdx-js/eslint-mdx/blob/master/package.json#L86-L88

@sheremet-va
Copy link
Member

sheremet-va commented Jan 4, 2024

I don't know how linter.js file works, so I cannot give any recommendations. All I can say is that supporting aliases and transformations in require is out of scope for Vitest. For aliases to apply, the file should be processed by Vitest, and the module should be imported using import. By default, all files outside of node_modules are processed. You have ./lib in your coverage because it was included somehow.

@sheremet-va
Copy link
Member

sheremet-va commented Jan 4, 2024

Also, it's possible that you have this file in your coverage because coverage includes all files under "include" by default. You can disable it with this config:

export default {
  test: {
    coverage: {
      all: false
    }
  }
}

Or you can also include only required files:

export default {
  test: {
    coverage: {
      include: ['./packages/*/src/**/*.ts']
    }
  }
}

@JounQin
Copy link
Author

JounQin commented Jan 4, 2024

The main issue is eslint will load the eslint-plugin-mdx and run some test cases, but if lib folder is resolved, the coverage reported will be totally unexpected, eslint related test cases will not increase the coverage because of the lib folder.

@sheremet-va
Copy link
Member

The issue with the coverage has nothing to do with resolve.alias. I checked that adding all: false to coverage options doesn't show any files that were not running during tests. I am closing the issue now.

@JounQin
Copy link
Author

JounQin commented Jan 5, 2024

@sheremet-va I don't quite understand what's your meaning.

@sheremet-va
Copy link
Member

@sheremet-va I don't quite understand what's your meaning.

Use this config options for coverage:

export default defineConfig({
  test: {
    coverage: {
      provider: 'istanbul',
      all: false,
    }
  }
})

It will only include files that you import in your tests.

@JounQin
Copy link
Author

JounQin commented Jan 5, 2024

image

@sheremet-va Is that really working as expected? Only the index.ts files are covered...

@sheremet-va
Copy link
Member

cc @AriPerkkio

@AriPerkkio
Copy link
Member

AriPerkkio commented Jan 5, 2024

Files that were loaded through Vite are present on coverage report. I guess ESLint loads your files using require?

Minimal reproduction would also be nice. I'm not quite sure what I'm looking at here.

@JounQin
Copy link
Author

JounQin commented Jan 5, 2024

I guess ESLint loads your files using require?

Yes.


OK, I'll try to provide a minimal reproduction.

@AriPerkkio
Copy link
Member

There's some discussion about coverage and require here:

But the main thing is that Vite intercepts only import and import(). Code that is run using require, child_process or worker_threads is not transformed. Vitest features like coverage and mocks won't work there.

@JounQin
Copy link
Author

JounQin commented Jan 5, 2024

Thanks for your references, I think I have to stick on jest for now.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants