Skip to content

refactor: adjust circularDependencyRspackPlugin compilation API#10006

Merged
fireairforce merged 4 commits intomainfrom
fix-9877
Apr 23, 2025
Merged

refactor: adjust circularDependencyRspackPlugin compilation API#10006
fireairforce merged 4 commits intomainfrom
fix-9877

Conversation

@fireairforce
Copy link
Contributor

@fireairforce fireairforce commented Apr 13, 2025 •
edited
Loading

Summary

In the method https://rspack.dev/zh/plugins/rspack/circular-dependency-rspack-plugin#ondetected , the compilation param is JsCompilation not Compilation in rust side(see docs: https://rspack.dev/api/javascript-api/compilation), which means:

image

I adjust the compilation inject time stage, just inject the compilation in the JS side, you can see my code at packages/src/builtin-plugin/CircularDependencyRspackPlugin.ts.

So the user can get the compilation api at js side:

image

closes issue: #9877

I add the case at packages/rspack-test-tools

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@github-actions github-actions bot added the release: document release: document related release(mr only) label Apr 13, 2025
@netlify
Copy link

netlify bot commented Apr 13, 2025 •
edited
Loading

✅ Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit 6009a9a
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/6808af8a1bd3460008836359

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 13, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #10006 will not alter performance

Comparing fix-9877 (6009a9a) with main (430ab91)

Summary

✅ 11 untouched benchmarks

@chenjiahan chenjiahan requested a review from SyMind April 13, 2025 07:33
chenjiahan
chenjiahan reviewed Apr 13, 2025
View reviewed changes
Copy link
Member

@chenjiahan chenjiahan left a comment

Choose a reason for hiding this comment

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

@SyMind should we expose JsCompilation to users?

@fireairforce
Copy link
Contributor Author

fireairforce commented Apr 13, 2025 •
edited
Loading

i am also curious about this, rspack don't export Compilation in crates/node_binding/binding.d.ts, so i have to use JsCompilation here(https://github.com/web-infra-dev/rspack/blob/main/crates/node_binding/binding.d.ts#L194).

@SyMind
Copy link
Member

SyMind commented Apr 14, 2025

I recommend implementing compilation.errors.push

@fireairforce
Copy link
Contributor Author

fireairforce commented Apr 17, 2025

ok, i will add this

@chenjiahan
Copy link
Member

chenjiahan commented Apr 17, 2025

@h-a-n-a I'm also curious if we should expose `compilation.pushDiagnostic' to users. It seems to be an internal API.

@h-a-n-a
Copy link
Contributor

h-a-n-a commented Apr 17, 2025 •
edited
Loading

@h-a-n-a I'm also curious if we should expose `compilation.pushDiagnostic' to users. It seems to be an internal API.

We've actually done this in loaderContext.experiments.emitDiagnostic and it's been used by an internal project in ByteDance. Exposing this will just need you to copy and paste some code here. It's totally a LGTM to me.

@chenjiahan
Copy link
Member

chenjiahan commented Apr 17, 2025

@h-a-n-a Cool, can you add documentation for loaderContext.experiments.emitDiagnostic first? 😄

@h-a-n-a h-a-n-a mentioned this pull request Apr 17, 2025
Closed
@fireairforce fireairforce force-pushed the fix-9877 branch 2 times, most recently from 64418d9 to 8a6e91c Compare April 23, 2025 08:51
@fireairforce fireairforce changed the title docs: update circularDependencyRspackplugin onDetected api refactor: adjust circularDependency compilation api Apr 23, 2025
@github-actions github-actions bot removed the release: document release: document related release(mr only) label Apr 23, 2025
@fireairforce
Copy link
Contributor Author

fireairforce commented Apr 23, 2025 •
edited
Loading

I just update the pr and don't use JsCompilation here anymore, more details please refer to the pr desc.

SyMind
SyMind previously approved these changes Apr 23, 2025
View reviewed changes
@fireairforce fireairforce changed the title refactor: adjust circularDependency compilation api refactor: adjust circularDependencyRspackPlugin compilation api Apr 23, 2025
chenjiahan
chenjiahan approved these changes Apr 23, 2025
View reviewed changes
Copy link
Member

@chenjiahan chenjiahan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@fireairforce fireairforce merged commit cc20f1a into main Apr 23, 2025
32 checks passed
@fireairforce fireairforce deleted the fix-9877 branch April 23, 2025 09:44
@chenjiahan chenjiahan changed the title refactor: adjust circularDependencyRspackPlugin compilation api refactor: adjust circularDependencyRspackPlugin compilation API Apr 23, 2025
@chenjiahan chenjiahan mentioned this pull request Apr 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chenjiahan chenjiahan chenjiahan approved these changes

@SyMind SyMind SyMind left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fireairforce @SyMind @chenjiahan @h-a-n-a