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(rspack_loader_swc): enable SWC plugins #5808

Merged
merged 2 commits into from
Mar 5, 2024
Merged

fix(rspack_loader_swc): enable SWC plugins #5808

merged 2 commits into from
Mar 5, 2024

Conversation

dm33tri
Copy link
Contributor

@dm33tri dm33tri commented Feb 28, 2024

Summary

Fixes #5763

I've locked rkyv to version which is used in swc and wrapped tuple in a struct as it wouldn't deserialize with an older rkyv version

Require Documentation?

  • No

@dm33tri dm33tri requested a review from hardfist as a code owner February 28, 2024 15:48
@CLAassistant
Copy link

CLAassistant commented Feb 28, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Feb 28, 2024
@dm33tri
Copy link
Contributor Author

dm33tri commented Feb 28, 2024

You may use this as a base or maybe I will add some test cases later

Also I had some runtime problems running pre-commit linters and didn't bother but will look into it

@h-a-n-a h-a-n-a self-assigned this Feb 29, 2024
@h-a-n-a
Copy link
Contributor

h-a-n-a commented Feb 29, 2024

Would you please add a case? ;-)

@dm33tri
Copy link
Contributor Author

dm33tri commented Feb 29, 2024

Would you please add a case? ;-)

I have added a test case

@dm33tri
Copy link
Contributor Author

dm33tri commented Feb 29, 2024

I didn't notice stats snapshot failing locally because my $HOME has . in it and it was replacing it with X and all stats snapshot tests failed. I've fixed this regexp:

// CHANGE: The time unit display in Rspack is second
.replace(/[.0-9]+(\s?s)/g, "X$1");

Removed old test, and updated snapshots

On my machine Diagnostics test set is still failing because it expects .js in stack traces but mine shows .ts

As for timeout of my test I don't know, will rerun fix it?

@dm33tri
Copy link
Contributor Author

dm33tri commented Feb 29, 2024

I guess I'll have to spin up a Linux VM or something, because on M1 it passes without any timeouts 😅 To check that it doesn't actually hang

And for CSS - is it just flaky, or did I actually break something?

@h-a-n-a
Copy link
Contributor

h-a-n-a commented Feb 29, 2024

I guess I'll have to spin up a Linux VM or something, because on M1 it passes without any timeouts 😅 To check that it doesn't actually hang

And for CSS - is it just flaky, or did I actually break something?

Yes, css test seems flaky and not being fixed. No worries. We will help you out!

Would you please take a look at your earliest convenience? @JSerFeng

@dm33tri
Copy link
Contributor Author

dm33tri commented Feb 29, 2024

Issue with the test looks similar to this swc-project/plugins#32 (comment)

@JSerFeng
Copy link
Contributor

the error _require(vm) called after all tests from css urls have completed

I guess the fix could be changing the tests/configCases/css/urls/index.js
to this:

const fs = require("fs");
const path = require("path");

it("css urls should works", async () => {
	await import("./urls.css");
	const css = await fs.promises.readFile(
		path.resolve(__dirname, "bundle.css"),
		"utf-8"
	);
	expect(css).toMatchSnapshot();
});

@JSerFeng
Copy link
Contributor

JSerFeng commented Feb 29, 2024

Also curious how large the @rspack/core package size increases to

@dm33tri
Copy link
Contributor Author

dm33tri commented Feb 29, 2024

Also curious how large the @rspack/core package size increases to

rspack.darwin-arm64.node in release mode is up to 45mb from 33mb

I guess the fix could be changing the tests/configCases/css/urls/index.js

I'm able to reproduce this error on different CSS test cases (with require(..) or import(..)) after cleaning up everything (so no caches). Will try few more times to see if it's my changes or not

@JSerFeng
Copy link
Contributor

JSerFeng commented Mar 1, 2024

I'm able to reproduce this error on different CSS test cases (with require(..) or import(..)) after cleaning up everything (so no caches). Will try few more times to see if it's my changes or not

If you have any trouble fixing that, feel free to tell us

@dm33tri
Copy link
Contributor Author

dm33tri commented Mar 1, 2024

I see that _require(vm) called after all tests is not from my code and it can flak in any test.

But I can't reproduce build hanging on mac/linux. Can I try to increase timeout for ConfigTestCases.template.js to 30s? Perhaps wasm runtime is just too slow in action runnner

● ConfigTestCases › builtin-swc-loader › swc-plugin › swc-plugin should compile
  thrown: "Exceeded timeout of 10000 ms for a test while waiting for `done()` to be called.

@dm33tri
Copy link
Contributor Author

dm33tri commented Mar 2, 2024

Seems like it helped

Would you approve please?

@hardfist hardfist requested a review from h-a-n-a March 4, 2024 10:54
@xc2
Copy link
Collaborator

xc2 commented Mar 5, 2024

!canary

Copy link
Contributor

github-actions bot commented Mar 5, 2024

0.5.5-canary-9e431a9-20240305043500

Copy link
Contributor

@h-a-n-a h-a-n-a left a comment

Choose a reason for hiding this comment

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

Thank you!

@h-a-n-a h-a-n-a merged commit a40b69d into web-infra-dev:main Mar 5, 2024
24 checks passed
h-a-n-a added a commit that referenced this pull request Mar 6, 2024
jerrykingxyz pushed a commit that referenced this pull request Mar 6, 2024
Revert "fix(rspack_loader_swc): enable SWC plugins (#5808)"

This reverts commit a40b69d.
@h-a-n-a h-a-n-a added the need documentation Create a tracking issue in rspack-website label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need documentation Create a tracking issue in rspack-website release: bug fix release: bug related release(mr only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: SWC plugins support
5 participants