feat: provide more friendly diagnostics if could not read path from a resolveId hook#1866
feat: provide more friendly diagnostics if could not read path from a resolveId hook#1866IWANABETHATGUY merged 10 commits intorolldown:mainfrom shulaoda:feat/friendly-diagnostic-for-unresolved-import
Conversation
✅ Deploy Preview for rolldown-rs canceled.
|
|
I think the pr still needs some refactoring and basic infra to be merged, it should not cost too much time. |
What should I do? What should be the expected effect? |
It is not related to your pull request, resolve ID should not be |
|
Also we lack the test infra to diagnostics in js side, if there is no test guaranteed, implementation is somehow meaningless(except it is a hotfix). |
<!-- Thank you for contributing! --> ### Description 1. replace `Arc<str>` with `ArcStr` 2. replace `String` with `ArcStr` 3. Normalize type related to module id related to #1866 <!-- Please insert your description here and provide especially info about the "what" this PR is solving -->
|
after #1871, you could acquire |
<!-- Thank you for contributing! --> ### Description 1. This pr add the ability to get the span of `module_request`. 2. Aiming to keep the type size unchanged, I did some refactors, If I add `Span` directly the type size will increase from `40` to `48` bytes. This time, I'm sticking with the current impl, since `8` bytes for each `RawImportRecord` is none trivial, think about it, for a medium-size project (10000 ~ 20000 modules with minimum import records only one), this could save about (80 ~ 150MB) memory. Related to #1866 <!-- Please insert your description here and provide especially info about the "what" this PR is solving -->
Let me start to try it. 😀 |
|
Due to my inability to intercept |
diff --git a/crates/rolldown/tests/rolldown/mod.rs b/crates/rolldown/tests/rolldown/mod.rs
index 9ef19e33..936bf634 100644
--- a/crates/rolldown/tests/rolldown/mod.rs
+++ b/crates/rolldown/tests/rolldown/mod.rs
@@ -1,2 +1,3 @@
mod errors;
mod issues;
+mod warnings;
diff --git a/crates/rolldown/tests/rolldown/warnings/mod.rs b/crates/rolldown/tests/rolldown/warnings/mod.rs
new file mode 100644
index 00000000..5da035d2
--- /dev/null
+++ b/crates/rolldown/tests/rolldown/warnings/mod.rs
@@ -0,0 +1,2 @@
+#[path = "./unloadable_import/mod.rs"]
+mod unloadable_import;
diff --git a/crates/rolldown/tests/rolldown/warnings/unloadable_import/artifacts.snap b/crates/rolldown/tests/rolldown/warnings/unloadable_import/artifacts.snap
new file mode 100644
index 00000000..303153af
--- /dev/null
+++ b/crates/rolldown/tests/rolldown/warnings/unloadable_import/artifacts.snap
@@ -0,0 +1,17 @@
+---
+source: crates/rolldown_testing/src/integration_test.rs
+---
+# Errors
+
+## UNRESOLVED_IMPORT
+
+```text
+[UNRESOLVED_IMPORT] Error: Could not resolve test.js
+ ╭─[entry.js:1:8]
+ │
+ 1 │ import 'test.js'
+ │ ────┬────
+ │ ╰────── No such file or directory (os error 2)
+───╯
+
+```
diff --git a/crates/rolldown/tests/rolldown/warnings/unloadable_import/entry.js b/crates/rolldown/tests/rolldown/warnings/unloadable_import/entry.js
new file mode 100644
index 00000000..0d98f1ed
--- /dev/null
+++ b/crates/rolldown/tests/rolldown/warnings/unloadable_import/entry.js
@@ -0,0 +1 @@
+import 'test.js'
diff --git a/crates/rolldown/tests/rolldown/warnings/unloadable_import/mod.rs b/crates/rolldown/tests/rolldown/warnings/unloadable_import/mod.rs
new file mode 100644
index 00000000..62029f1d
--- /dev/null
+++ b/crates/rolldown/tests/rolldown/warnings/unloadable_import/mod.rs
@@ -0,0 +1,53 @@
+use std::{borrow::Cow, sync::Arc};
+
+use rolldown::{BundlerOptions, InputItem};
+use rolldown_plugin::{
+ HookResolveIdArgs, HookResolveIdOutput, HookResolveIdReturn, Plugin, SharedPluginContext,
+};
+use rolldown_testing::{abs_file_dir, integration_test::IntegrationTest, test_config::TestMeta};
+
+#[derive(Debug)]
+struct UnLoadableImport;
+
+impl Plugin for UnLoadableImport {
+ fn name(&self) -> Cow<'static, str> {
+ "unloadable-import".into()
+ }
+
+ async fn resolve_id(
+ &self,
+ _ctx: &SharedPluginContext,
+ args: &HookResolveIdArgs<'_>,
+ ) -> HookResolveIdReturn {
+ if args.specifier == "test.js" {
+ return Ok(Some(HookResolveIdOutput {
+ id: args.specifier.to_string(),
+ ..Default::default()
+ }));
+ }
+ Ok(None)
+ }
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn should_emit_diagnostic_when_could_not_load_dependency() {
+ let cwd = abs_file_dir!();
+
+ IntegrationTest::new(TestMeta {
+ expect_executed: false,
+ expect_error: true,
+ ..Default::default()
+ })
+ .run_with_plugins(
+ BundlerOptions {
+ input: Some(vec![InputItem {
+ name: Some("entry".to_string()),
+ import: "./entry.js".to_string(),
+ }]),
+ cwd: Some(cwd),
+ ..Default::default()
+ },
+ vec![Arc::new(UnLoadableImport)],
+ )
+ .await;
+} |
|
Can you patch this rust test, and revert all modifications on the js side? The new JS tests seems meaningless. |
crates/rolldown/tests/rolldown/errors/unresolved_import/unknown_module_type_entry/mod.rs
Outdated
Show resolved
Hide resolved
...es/rolldown/tests/rolldown/errors/unresolved_import/unknown_module_type_entry/artifacts.snap
Show resolved
Hide resolved
crates/rolldown/tests/rolldown/errors/unresolved_import/unknown_module_type/mod.rs
Outdated
Show resolved
Hide resolved
|
Overall, LGTM, but I prefer the |
Yeah. A new error would be more proper. |
Head branch was pushed to by a user without write access
|
Thanks |
Description
Closes #1851
I don't know if it should be simply regarded as a
rolldown internal error.At first, I tried to implement the incorrect format of
esbuild, but later I found thatrolldownis not enough to support me in doing so.Therefore, I attempted to implement the incorrect format of
rollup.Due to the current lack of support for plugins in
rolldown testing, I am unable to conduct testing onrust. In addition, testing infixturescan lead tobuild errors, so currently I can only ignore it.