Skip to content

feat: emit warning for circular dependency#839

Merged
hyf0 merged 4 commits into
rolldown:mainfrom
PengBoUESTC:feature/emit-warnings-for-circular-dependencies
Apr 12, 2024
Merged

feat: emit warning for circular dependency#839
hyf0 merged 4 commits into
rolldown:mainfrom
PengBoUESTC:feature/emit-warnings-for-circular-dependencies

Conversation

@PengBoUESTC

Copy link
Copy Markdown
Contributor

Description

emit warning for circular dependency

for #831

@netlify

netlify Bot commented Apr 12, 2024

Copy link
Copy Markdown

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 15b5021
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/6619305b7166110008a0894f

Comment thread crates/rolldown_error/src/events/circular_dependency.rs
@@ -97,7 +98,27 @@ impl<'a> LinkStage<'a> {
if !circular_dependencies.is_empty() {
let mut cycles = circular_dependencies.into_iter().collect::<Vec<_>>();
cycles.sort();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove this sort call. moduleid is not stable.

self.warnings.extend(cycles.iter().map(|path| {
let module_names: Vec<String> = path
.iter()
.map(|id| match id {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Filter out external module id.

ModuleId::Normal(id) => self
.module_table
.normal_modules
.get(*id)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't use get, use []

.map_or_else(|| "Unknown external module".to_string(), |module| module.name.clone()),
ModuleId::Normal(id) => self.module_table.normal_modules.get(*id).map_or_else(
|| "Unknown normal module".to_string(),
|module| module.pretty_path.clone(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use resource id

}

fn message(&self) -> String {
format!("Circular dependency: {}.", self.paths.join(" -> "))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Call relative display here

@codspeed-hq

codspeed-hq Bot commented Apr 12, 2024

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #839 will not alter performance

Comparing PengBoUESTC:feature/emit-warnings-for-circular-dependencies (15b5021) with main (ba6b79d)

Summary

✅ 6 untouched benchmarks

@hyf0 hyf0 linked an issue Apr 12, 2024 that may be closed by this pull request
@hyf0 hyf0 self-assigned this Apr 12, 2024
@hyf0 hyf0 force-pushed the feature/emit-warnings-for-circular-dependencies branch from 251e608 to 1bc0c51 Compare April 12, 2024 12:48
@hyf0

hyf0 commented Apr 12, 2024

Copy link
Copy Markdown
Member

Need to something with logging. Let's me take this PR from here. You could leave it now.

@hyf0 hyf0 enabled auto-merge (squash) April 12, 2024 13:00

@hyf0 hyf0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@hyf0 hyf0 merged commit 6d64a64 into rolldown:main Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Emit warnings for circular dependencies

2 participants