Skip to content

Commit

Permalink
fix: should generate correct chunk import path to importee in importe…
Browse files Browse the repository at this point in the history
…r chunk
  • Loading branch information
hyf0 committed May 20, 2024
1 parent 9204e6d commit 3e815f7
Show file tree
Hide file tree
Showing 17 changed files with 158 additions and 51 deletions.
2 changes: 2 additions & 0 deletions crates/rolldown/src/finalizer/finalizer_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::{
linking_metadata::{LinkingMetadata, LinkingMetadataVec},
symbols::Symbols,
},
SharedOptions,
};

pub struct FinalizerContext<'me> {
Expand All @@ -22,4 +23,5 @@ pub struct FinalizerContext<'me> {
pub canonical_names: &'me FxHashMap<SymbolRef, Rstr>,
pub runtime: &'me RuntimeModuleBrief,
pub chunk_graph: &'me ChunkGraph,
pub options: &'me SharedOptions,
}
15 changes: 10 additions & 5 deletions crates/rolldown/src/finalizer/impl_visit_mut_for_finalizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,12 +412,17 @@ impl<'me, 'ast> VisitMut<'ast> for Finalizer<'me, 'ast> {
let importee_id = rec.resolved_module;
match importee_id {
ModuleId::Normal(importee_id) => {
let chunk_id = self.ctx.chunk_graph.module_to_chunk[importee_id]
let importer_chunk_id = self.ctx.chunk_graph.module_to_chunk[self.ctx.module.id]
.expect("Normal module should belong to a chunk");
let chunk = &self.ctx.chunk_graph.chunks[chunk_id];
str.value = self
.snippet
.atom(&format!("./{}", &**chunk.preliminary_filename.as_deref().unwrap()));
let importer_chunk = &self.ctx.chunk_graph.chunks[importer_chunk_id];

let importee_chunk_id = self.ctx.chunk_graph.module_to_chunk[importee_id]
.expect("Normal module should belong to a chunk");
let importee_chunk = &self.ctx.chunk_graph.chunks[importee_chunk_id];

let import_path = importer_chunk.import_path_for(importee_chunk);

str.value = self.snippet.atom(&import_path);
}
ModuleId::External(_) => {
// external module doesn't belong to any chunk, just keep this as it is
Expand Down
8 changes: 7 additions & 1 deletion crates/rolldown/src/stages/generate_stage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ use rolldown_common::{
};
use rolldown_error::BuildError;
use rolldown_plugin::SharedPluginDriver;
use rolldown_utils::rayon::{ParallelBridge, ParallelIterator};
use rolldown_utils::{
path_buf_ext::PathBufExt,
rayon::{ParallelBridge, ParallelIterator},
};
use sugar_path::SugarPath;

use crate::{
Expand Down Expand Up @@ -82,6 +85,7 @@ impl<'a> GenerateStage<'a> {
linking_infos: &self.link_output.metas,
runtime: &self.link_output.runtime,
chunk_graph: &chunk_graph,
options: self.options,
},
ast,
);
Expand Down Expand Up @@ -281,6 +285,8 @@ impl<'a> GenerateStage<'a> {
hash: hash_placeholder.as_deref(),
});

chunk.absolute_preliminary_filename =
Some(preliminary.absolutize_with(&self.options.dir).expect_into_string());
chunk.preliminary_filename = Some(PreliminaryFilename::new(preliminary, hash_placeholder));
});
}
Expand Down
11 changes: 4 additions & 7 deletions crates/rolldown/src/utils/chunk/render_chunk_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,18 +71,15 @@ pub fn render_chunk_imports(
render_import_specifier(export_alias, local_binding)
})
.collect::<Vec<_>>();
let filename = importee_chunk
.preliminary_filename
.as_deref()
.expect("At this point, preliminary_filename should already be generated")
.as_str();

let import_path = chunk.import_path_for(importee_chunk);

if import_items.is_empty() {
// TODO: filename relative to importee
render_plain_import(&format!("./{filename}"), &mut s);
render_plain_import(&import_path, &mut s);
} else {
import_items.sort();
render_import_stmt(&import_items, &format!("./{filename}"), &mut s);
render_import_stmt(&import_items, &import_path, &mut s);
}
});

Expand Down
70 changes: 37 additions & 33 deletions crates/rolldown/tests/common/fixture.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,42 +41,46 @@ impl Fixture {
pub fn exec(&self) {
let test_config = self.test_config();

if !test_config.expect_executed || test_config.expect_error {
return;
}
let mut command = Command::new("node");

let dist_folder = self.dir_path().join("dist");
let test_script = self.dir_path().join("_test.mjs");

let is_output_cjs = matches!(test_config.config.format, Some(OutputFormat::Cjs));

let compiled_entries = test_config
.config
.input
.unwrap_or_else(|| vec![default_test_input_item()])
.iter()
.map(|item| {
let name = item.name.clone().expect("inputs must have `name` in `_config.json`");
let ext = if is_output_cjs { "cjs" } else { "mjs" };
format!("{name}.{ext}",)
})
.map(|name| dist_folder.join(name))
.collect::<Vec<_>>();

let mut command = Command::new("node");
compiled_entries.iter().for_each(|entry| {
if is_output_cjs {
command.arg("--require");
} else {
command.arg("--import");
}
if cfg!(target_os = "windows") && !is_output_cjs {
// Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs.
command.arg(format!("file://{}", entry.to_str().expect("should be valid utf8")));
} else {
command.arg(entry);
}
});
if !test_config.expect_executed || test_config.expect_error {
// do nothing
} else {
// Notices, we now don't have the finalized `dir` value, so we assume the `dist` folder is the output folder. But this cause
// problem once `entry_filenames` or `dir` is configured using a different folder.
let dist_folder = self.dir_path().join("dist");

let is_output_cjs = matches!(test_config.config.format, Some(OutputFormat::Cjs));

let compiled_entries = test_config
.config
.input
.unwrap_or_else(|| vec![default_test_input_item()])
.iter()
.map(|item| {
let name = item.name.clone().expect("inputs must have `name` in `_config.json`");
let ext = if is_output_cjs { "cjs" } else { "mjs" };
format!("{name}.{ext}",)
})
.map(|name| dist_folder.join(name))
.collect::<Vec<_>>();

compiled_entries.iter().for_each(|entry| {
if is_output_cjs {
command.arg("--require");
} else {
command.arg("--import");
}
if cfg!(target_os = "windows") && !is_output_cjs {
// Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs.
command.arg(format!("file://{}", entry.to_str().expect("should be valid utf8")));
} else {
command.arg(entry);
}
});
}

if test_script.exists() {
command.arg(test_script);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
{
"_comment": "`sideEffects` in `shared.js` is not defined, so the input is invalid in the first place.",
"expectExecuted": false,
"config": {
"input": [
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"config": {
"input": [
{
"name": "a",
"import": "./a.js"
},
{
"name": "b",
"import": "./b.js"
}
],
"entryFilenames": "./entries/[name].mjs",
"chunkFilenames": "./chunks/[name].mjs"
},
"_comment": "See comments in `crates/rolldown/tests/common/fixture.rs`",
"expectExecuted": false
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import assert from 'node:assert';
import { value, asyncValue } from './dist/entries/a.mjs'
import { value as value2, asyncValue as asyncValue2 } from './dist/entries/b.mjs'

assert.strictEqual(value, value2);
assert.strictEqual(await asyncValue, await asyncValue2);
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { value } from './shared.js'
export const asyncValue = import('./async.js')
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
---
source: crates/rolldown/tests/common/case.rs
expression: content
input_file: crates/rolldown/tests/fixtures/function/entry_filenames/should_generate_correct_relative_import_path
---
# Assets

## ./chunks/async.mjs

```js
// async.js
const value = 'async';

export { value };
```
## ./chunks/shared.mjs
```js
// shared.js
const value = 'shared';

export { value };
```
## ./entries/a.mjs
```js
import { value } from "../chunks/shared.mjs";

// a.js
const asyncValue = import('../chunks/async.mjs');

export { asyncValue, value };
```
## ./entries/b.mjs
```js
import { value } from "../chunks/shared.mjs";

// b.js
const asyncValue = import('../chunks/async.mjs');

export { asyncValue, value };
```
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const value = 'async'
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { value } from './shared.js'
export const asyncValue = import('./async.js')
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const value = 'shared'
Original file line number Diff line number Diff line change
Expand Up @@ -1225,6 +1225,13 @@ expression: "snapshot_outputs.join(\"\\n\")"
# tests/fixtures/errors/unresolved_entry


# tests/fixtures/function/entry_filenames/should_generate_correct_relative_import_path

- ./chunks/async.mjs => ./chunks/async.mjs
- ./chunks/shared.mjs => ./chunks/shared.mjs
- ./entries/a.mjs => ./entries/a.mjs
- ./entries/b.mjs => ./entries/b.mjs

# tests/fixtures/function/external/export_external

- main-!~{000}~.mjs => main-dVl9dQVv.mjs
Expand Down
17 changes: 16 additions & 1 deletion crates/rolldown_common/src/chunk/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ use crate::{
pub mod types;

use rolldown_rstr::Rstr;
use rolldown_utils::BitSet;
use rolldown_utils::{path_ext::PathExt, BitSet};
use rustc_hash::FxHashMap;
use sugar_path::SugarPath;

use self::types::{
cross_chunk_import_item::CrossChunkImportItem, preliminary_filename::PreliminaryFilename,
Expand All @@ -20,6 +21,7 @@ pub struct Chunk {
pub name: Option<String>,
pub filename: Option<ResourceId>,
pub preliminary_filename: Option<PreliminaryFilename>,
pub absolute_preliminary_filename: Option<String>,
pub canonical_names: FxHashMap<SymbolRef, Rstr>,
// Sorted by resource_id of modules in the chunk
pub cross_chunk_imports: Vec<ChunkId>,
Expand Down Expand Up @@ -61,4 +63,17 @@ impl Chunk {
// Now we just return `true`
true
}

pub fn import_path_for(&self, importee: &Chunk) -> String {
let importer_dir =
self.absolute_preliminary_filename.as_ref().unwrap().as_path().parent().unwrap();
let importee_filename = importee.absolute_preliminary_filename.as_ref().unwrap();
let import_path = importee_filename.relative(importer_dir).as_path().expect_to_slash();

if import_path.starts_with('.') {
import_path
} else {
format!("./{import_path}")
}
}
}
2 changes: 1 addition & 1 deletion crates/rolldown_testing/_config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"type": "boolean"
},
"expectExecuted": {
"description": "If `false`, the compiled artifacts won't be executed.",
"description": "If `false`, the compiled artifacts won't be executed, but `_test.mjs` will be still executed if exists.",
"default": true,
"type": "boolean"
},
Expand Down
2 changes: 1 addition & 1 deletion crates/rolldown_testing_config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ pub struct TestConfig {
#[serde(default)]
pub config: rolldown_common::BundlerOptions,
#[serde(default = "true_by_default")]
/// If `false`, the compiled artifacts won't be executed.
/// If `false`, the compiled artifacts won't be executed, but `_test.mjs` will be still executed if exists.
pub expect_executed: bool,
#[serde(default)]
/// If `true`, the fixture are expected to fail to compile/build.
Expand Down

0 comments on commit 3e815f7

Please sign in to comment.