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

Use Vitest instead of Jest #288

Merged
merged 6 commits into from
Nov 13, 2024
Merged

Use Vitest instead of Jest #288

merged 6 commits into from
Nov 13, 2024

Conversation

siketyan
Copy link
Contributor

@siketyan siketyan commented Nov 7, 2024

Closes #263

This pull request replaces Jest with Vitest.

At the first try, I found that Vitest does not resolve await-lock module correctly.
The package is not maintained recently and has an issue with ESM interop.
I replaced it with async-mutex to resolve the issue.

Signed-off-by: Naoki Ikeguchi <naoki.ikeguchi@bitkey.jp>
Signed-off-by: Naoki Ikeguchi <naoki.ikeguchi@bitkey.jp>
@mizdra
Copy link
Owner

mizdra commented Nov 13, 2024

Thank you for doing this work! I will accept and use this Pull Request.

At the first try, I found that Vitest does not resolve await-lock module correctly.
The package is not maintained recently and has an issue with ESM interop.
I replaced it with async-mutex to resolve the issue.

I don't object to replacing it with async-mutex. However, I don't think that the package is not maintained. The maintainer replied to the issue comment in August 2023. It seems that the package is maintained.

@mizdra
Copy link
Owner

mizdra commented Nov 13, 2024

Should we also delete the following line?

Signed-off-by: Naoki Ikeguchi <naoki.ikeguchi@bitkey.jp>
Signed-off-by: Naoki Ikeguchi <naoki.ikeguchi@bitkey.jp>
Signed-off-by: Naoki Ikeguchi <naoki.ikeguchi@bitkey.jp>
@siketyan
Copy link
Contributor Author

siketyan commented Nov 13, 2024

@mizdra
Sorry for my miswording. I meant that await-lock is not shipped with native ESM support, breaking the type definition of the default import.

@siketyan siketyan requested a review from mizdra November 13, 2024 02:28
- Vitest outputted `warning: The "??" operator here will always return the left operand`
@mizdra
Copy link
Owner

mizdra commented Nov 13, 2024

When I run npm run test -- runner.test.ts, Vite seems to output the following warning

$ npm run test -- runner.test.ts
...
1:17:09 AM [vite] Failed to load source map for /Users/mizdra/src/github.com/mizdra/happy-css-modules/packages/happy-css-modules/src/runner.test.ts.
Error: An error occurred while trying to read the map file at ./1.css.d.ts.map
Error: ENOENT: no such file or directory, open '/Users/mizdra/src/github.com/mizdra/happy-css-modules/packages/happy-css-modules/src/1.css.d.ts.map'
    at async open (node:internal/fs/promises:638:25)
    at async Object.readFile (node:internal/fs/promises:1238:14)
    at async extractSourcemapFromFile (file:///Users/mizdra/src/github.com/mizdra/happy-css-modules/node_modules/vite/dist/node/chunks/dep-BWSbWtLw.js:21001:53)
    at async loadAndTransform (file:///Users/mizdra/src/github.com/mizdra/happy-css-modules/node_modules/vite/dist/node/chunks/dep-BWSbWtLw.js:51892:27)
    at async ViteNodeServer._transformRequest (file:///Users/mizdra/src/github.com/mizdra/happy-css-modules/node_modules/vite-node/dist/server.mjs:537:16)
    at async ViteNodeServer._fetchModule (file:///Users/mizdra/src/github.com/mizdra/happy-css-modules/node_modules/vite-node/dist/server.mjs:499:17)
    at async Proxy.fetch (file:///Users/mizdra/src/github.com/mizdra/happy-css-modules/node_modules/vitest/dist/chunks/resolveConfig.DPmbhVlP.js:6616:22)
    at async EventEmitter.onMessage (file:///Users/mizdra/src/github.com/mizdra/happy-css-modules/node_modules/vitest/dist/chunks/index.68735LiX.js:91:20)
...

Apparently, Vitest found the following line and misunderstood it to indicate the source map for runner.test.ts. I believe this is a bug in Vitest 😆.

//# sourceMappingURL=./1.css.d.ts.map

The following line seems to be causing the bug. It should be /^\/\/[@#] instead of /\/\/[@#].

https://github.com/vitest-dev/vitest/blob/85c64e35b120f240e66d5fb9ca67fa55ed8eb74d/packages/vite-node/src/source-map-handler.ts#L116

@mizdra
Copy link
Owner

mizdra commented Nov 13, 2024

Let's ignore the Vitest bug here. If you are interested in reporting this bug to Vitest team, please do so. If you are not interested, I will do it for you.

Copy link
Owner

@mizdra mizdra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for contributing!

@mizdra mizdra merged commit e9324dd into mizdra:main Nov 13, 2024
6 checks passed
@mizdra mizdra changed the title feat: Use Vitest instead of Jest Use Vitest instead of Jest Nov 13, 2024
@mizdra mizdra added the Type: Other Cannot be classified into any other type. label Nov 13, 2024
@mizdra
Copy link
Owner

mizdra commented Nov 13, 2024

The following line seems to be causing the bug. It should be /^//[@#] instead of ///[@#].

This may be my misunderstanding. We need to check https://tc39.es/source-map/ to determine if this is a bug.

@siketyan
Copy link
Contributor Author

@mizdra I checked the spec and found that whitespaces before the sourceMappingURL comment are allowed, as defined in https://tc39.es/source-map/#extraction-javascript. Thus, I think it's an expected behaviour and we cannot declare this as a Vitest's bug.

@siketyan
Copy link
Contributor Author

And, this example is similar to our situation: https://tc39.es/source-map/#example-5b7659a8. This type of code can be parsed as unintentional even if the regexp starts with ^, unless Vitest parses it using AST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Other Cannot be classified into any other type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Vitest intead of Jest
2 participants