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

Fix moduleNameMapper to not resolve .wasm.js to .wasm #9894

Merged
merged 20 commits into from
Feb 12, 2023

Conversation

saschanaz
Copy link
Member

@saschanaz saschanaz commented Feb 11, 2023

What

  1. Added a rule for *.wasm.js so that the existing rule won't change its path. Also added comments.
  2. Removed the custom resolver because [Feature]: Support package imports in jest-resolve jestjs/jest#12270 is solved.
  1. 既存のルールが*.wasm.jsのpathを変えないようにルールを追加しました。コメントも書きました。
  2. [Feature]: Support package imports in jest-resolve jestjs/jest#12270 は解決づみなのでcustom resolverは削除しました。

Why

Undici tries to import ./llhttp/llhttp.wasm.js which is currently broken by the (hacky) moduleNameMapper.

Undiciの ./llhttp/llhttp.wasm.js importを moduleNameMapperが邪魔しています。

Additional info (optional)

Fixes #9767

Fixes misskey-dev#9767

Undici [tries to import `./llhttp/llhttp.wasm.js`](https://github.com/nodejs/undici/blob/e155c6db5cec9bc577d548fa7c7378013631c79c/lib/client.js#L342) which is currently broken by the (hacky) module name mapper.
@github-actions github-actions bot added the packages/backend Server side specific issue/PR label Feb 11, 2023
@saschanaz saschanaz changed the title Fix moduleNameMapper to not resolve .wasm.js to .js Fix moduleNameMapper to not resolve .wasm.js to .wasm Feb 11, 2023
@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Merging #9894 (37c2f2e) into develop (b2ed4c9) will increase coverage by 0.34%.
The diff coverage is 19.42%.

❗ Current head 37c2f2e differs from pull request most recent head bff0bee. Consider uploading reports for the commit bff0bee to get more accurate results

@@             Coverage Diff             @@
##           develop    #9894      +/-   ##
===========================================
+ Coverage    22.53%   22.87%   +0.34%     
===========================================
  Files          735      729       -6     
  Lines        68917    67998     -919     
  Branches      2022     2070      +48     
===========================================
+ Hits         15527    15557      +30     
+ Misses       53390    52441     -949     
Impacted Files Coverage Δ
packages/backend/src/core/CoreModule.ts 100.00% <ø> (ø)
packages/backend/src/core/CustomEmojiService.ts 35.60% <0.00%> (ø)
packages/backend/src/core/DownloadService.ts 30.76% <0.00%> (-0.24%) ⬇️
packages/backend/src/core/DriveService.ts 20.77% <0.00%> (-0.22%) ⬇️
packages/backend/src/core/HashtagService.ts 20.68% <ø> (-0.51%) ⬇️
packages/backend/src/core/HttpRequestService.ts 49.37% <0.00%> (ø)
packages/backend/src/core/WebhookService.ts 40.47% <0.00%> (-4.86%) ⬇️
.../backend/src/core/activitypub/ApRendererService.ts 31.61% <0.00%> (ø)
...backend/src/core/activitypub/LdSignatureService.ts 26.92% <0.00%> (-0.53%) ⬇️
...kend/src/core/activitypub/models/ApImageService.ts 48.45% <0.00%> (-2.09%) ⬇️
... and 85 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions github-actions bot removed the 🧪Test label Feb 11, 2023
@saschanaz
Copy link
Member Author

ローカルではこれでいけるのになんででしょう。ELIFECYCLEの謎。

#9767 の警告自体はこれで解決できるから、あれはまた別のPRで考えてみましょう。

@@ -85,7 +84,8 @@ class LdSignature {
@bindThis
public async normalize(data: any) {
const customLoader = this.getLoader();
return await jsonld.normalize(data, {
// XXX: Importing jsonld dynamically since Jest frequently fails to import it statically
Copy link
Member Author

@saschanaz saschanaz Feb 12, 2023

Choose a reason for hiding this comment

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

諦めた結果こうなってしまいました
なんかjsonldのdependencyである@digitalbazaar/http-client/dist/cjs/index.cjsのdynamic importあたりでなにかエラーが出てくるみたいですが、原因不明です。

Copy link
Member Author

Choose a reason for hiding this comment

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

jestjs/jest#10620 (comment) 関係あるかもです、ないかもです

When I call it with a valid filepath, Jest exits with a non-zero exit code without reporting any failure (I would expect it to throw "Ok 2").

まさにその通り

Copy link
Member Author

Choose a reason for hiding this comment

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

そもそもCJSとかあるnodeが悪い、denoにするべき💢

@syuilo syuilo merged commit 9965bc8 into misskey-dev:develop Feb 12, 2023
@syuilo
Copy link
Member

syuilo commented Feb 12, 2023

🙏🏻🙏🏻🙏🏻

@saschanaz saschanaz deleted the fix-jest branch February 12, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend Server side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ユニットテストで警告が出る
2 participants