Skip to content

Fix/context module source with first empty quasis #1719

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.

Sign up for GitHub

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

Merged
stormslowly merged 2 commits into from
Dec 12, 2024
Merged

Fix/context module source with first empty quasis #1719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f87b898
test: ✅ add first empty quasis template string module source case
stormslowly Dec 11, 2024
7218565
fix: 🐛 first emtpy quasis case
stormslowly Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 32 additions & 17 deletions crates/mako/src/plugins/context_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::collections::{BTreeMap, HashSet};
use std::fs;
use std::path::Path;
use std::sync::Arc;
Expand Down Expand Up @@ -38,25 +39,34 @@
let glob_pattern = param.file.pathname.clone().join(glob_pattern);
let paths = glob(glob_pattern.to_str().unwrap())?;

let mut key_values = vec![];
let mut key_values = BTreeMap::new();
let load_by = if param.file.has_param("async") {
"import"

Check warning on line 44 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L42-L44 

Added lines #L42 - L44 were not covered by tests
} else {
"require"

Check warning on line 46 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L46 

Added line #L46 was not covered by tests
};

for path in paths {
let path = path?;
let rlt_path = path.strip_prefix(&param.file.pathname)?;
let is_file = path.is_file();

Check warning on line 52 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L52 

Added line #L52 was not covered by tests

// full path `./i18n/jzh_CN.json`
let mut keys = HashSet::new();

Check warning on line 55 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L55 

Added line #L55 was not covered by tests

// full path `./i18n/zh_CN.json`
let mut keys: Vec<String> = vec![];
let metadata = fs::metadata(&path);
if let Ok(md) = metadata {
if md.is_dir() && !has_index_file_in_directory(&path) {
continue;
}
}
keys.push(format!("./{}", rlt_path.to_string_lossy()));
keys.insert(format!("./{}", rlt_path.to_string_lossy()));

Check warning on line 63 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L63 

Added line #L63 was not covered by tests
// omit ext `./i18n/zh_CN`
if let Some(ext) = rlt_path.extension() {
if get_module_extensions().contains(&format!(".{}", ext.to_string_lossy())) {
keys.push(format!(
if is_file
&& get_module_extensions().contains(&format!(".{}", ext.to_string_lossy()))

Check warning on line 67 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L66-L67 

Added lines #L66 - L67 were not covered by tests
{
keys.insert(format!(

Check warning on line 69 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L69 

Added line #L69 was not covered by tests
"./{}",
rlt_path.with_extension("").to_string_lossy()
));
Expand Down Expand Up @@ -85,16 +95,11 @@
}
}

let is_async = param.file.has_param("async");

for key in keys {
let load_by = if is_async { "import" } else { "require" };
key_values.push(format!(
"'{}': () => {}('{}')",
key,
load_by,
path.to_string_lossy()
));
let map_entry =
format!("'{}': () => {}('{}')", key, load_by, path.to_string_lossy());

Check warning on line 100 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L100 

Added line #L100 was not covered by tests

key_values.insert(key, map_entry);

Check warning on line 102 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L102 

Added line #L102 was not covered by tests
}
}

Expand All @@ -113,7 +118,10 @@
}}
}};
"#,
key_values.join(",\n")
key_values

Check warning on line 121 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L121 

Added line #L121 was not covered by tests
.into_values()
.collect::<Vec<String>>()
.join(",\n")
);
Ok(Some(Content::Js(JsContent {
content,
Expand Down Expand Up @@ -245,9 +253,16 @@
}

// handle `./foo/${bar}.ext`
// `${bar}` will be handle as `./${bar}`
Expr::Tpl(tpl) => {
if !tpl.exprs.is_empty() {
let pre_quasis = tpl.quasis.first().unwrap().raw.to_string();
let first_quasis_str = tpl.quasis.first().unwrap().raw.to_string();
let pre_quasis = if first_quasis_str.is_empty() {
"./".to_string()

Check warning on line 261 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L259-L261 

Added lines #L259 - L261 were not covered by tests
} else {
first_quasis_str

Check warning on line 263 in crates/mako/src/plugins/context_module.rs

View check run for this annotation

Codecov / codecov/patch

crates/mako/src/plugins/context_module.rs#L263 

Added line #L263 was not covered by tests
};

Comment on lines +256 to +265
Copy link

@coderabbitai coderabbitai bot Dec 11, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议改进模板字符串处理逻辑

当前的模板字符串处理逻辑可能存在边缘情况未处理：

  1. 需要验证 quasis 数组是否为空
  2. 建议添加更多的错误处理
 Expr::Tpl(tpl) => {
     if !tpl.exprs.is_empty() {
-        let first_quasis_str = tpl.quasis.first().unwrap().raw.to_string();
+        let first_quasis_str = tpl.quasis.first()
+            .ok_or_else(|| anyhow::anyhow!("Template literal must have at least one quasi"))?
+            .raw.to_string();
         let pre_quasis = if first_quasis_str.is_empty() {
             "./".to_string()
         } else {
             first_quasis_str
         };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// `${bar}` will be handle as `./${bar}`
Expr::Tpl(tpl) => {
if !tpl.exprs.is_empty() {
let pre_quasis = tpl.quasis.first().unwrap().raw.to_string();
let first_quasis_str = tpl.quasis.first().unwrap().raw.to_string();
let pre_quasis = if first_quasis_str.is_empty() {
"./".to_string()
} else {
first_quasis_str
};
// `${bar}` will be handle as `./${bar}`
Expr::Tpl(tpl) => {
if !tpl.exprs.is_empty() {
let first_quasis_str = tpl.quasis.first()
.ok_or_else(|| anyhow::anyhow!("Template literal must have at least one quasi"))?
.raw.to_string();
let pre_quasis = if first_quasis_str.is_empty() {
"./".to_string()
} else {
first_quasis_str
};

let (prefix, remainder) = if let Some(pos) = pre_quasis.rfind('/') {
(
pre_quasis[..=pos].to_string(),
Expand Down
60 changes: 33 additions & 27 deletions ...ures/javascript.require-dynamic/expect.js → ...tures/javascript.context-module/expect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const assert = require("assert");
const { parseBuildResult, moduleReg } = require("../../../scripts/test-utils");
const { files } = parseBuildResult(__dirname);
const {parseBuildResult, moduleReg} = require("../../../scripts/test-utils");
const {files} = parseBuildResult(__dirname);

const names = Object.keys(files);
const content = files["index.js"];
Expand All @@ -11,26 +11,30 @@ assert.match(
moduleReg(
"src\\?context&glob=\\*\\*/\\*",
[
"'./ext/json.ts': ()=>__mako_require__(\"src/ext/json.ts\")",
"'./ext/json': ()=>__mako_require__(\"src/ext/json.ts\")",
"'./fake.js': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./fake': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./fake.js/a.js': ()=>__mako_require__(\"src/fake.js/a.js\")",
"'./fake.js/a': ()=>__mako_require__(\"src/fake.js/a.js\")",
"'./fake.js/aa.js': ()=>__mako_require__(\"src/fake.js/aa.js\")",
"'./fake.js/aa': ()=>__mako_require__(\"src/fake.js/aa.js\")",
"'./fake.js/index.js': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./fake.js/index': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./fake.js': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./fake.js/': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./i18n/en-US.json': ()=>__mako_require__(\"src/i18n/en-US.json\")",
"'./i18n/en-US': ()=>__mako_require__(\"src/i18n/en-US.json\")",
"'./i18n/zh-CN.json': ()=>__mako_require__(\"src/i18n/zh-CN.json\")",
"'./i18n/zh-CN': ()=>__mako_require__(\"src/i18n/zh-CN.json\")",
"'./index.ts': ()=>__mako_require__(\"src/index.ts\")",
"'./index': ()=>__mako_require__(\"src/index.ts\")",
"'.': ()=>__mako_require__(\"src/index.ts\")",
"'./': ()=>__mako_require__(\"src/index.ts\")"
`'.': ()=>__mako_require__("src/index.ts")`,
`'./': ()=>__mako_require__("src/index.ts")`,
`'./ext/json': ()=>__mako_require__("src/ext/json.ts")`,
`'./ext/json.ts': ()=>__mako_require__("src/ext/json.ts")`,
`'./fake.js': ()=>__mako_require__("src/fake.js/index.js")`,
`'./fake.js/': ()=>__mako_require__("src/fake.js/index.js")`,
`'./fake.js/a': ()=>__mako_require__("src/fake.js/a.js")`,
`'./fake.js/a.js': ()=>__mako_require__("src/fake.js/a.js")`,
`'./fake.js/aa': ()=>__mako_require__("src/fake.js/aa.js")`,
`'./fake.js/aa.js': ()=>__mako_require__("src/fake.js/aa.js")`,
`'./fake.js/index': ()=>__mako_require__("src/fake.js/index.js")`,
`'./fake.js/index.js': ()=>__mako_require__("src/fake.js/index.js")`,
`'./first_empty_quasis': ()=>__mako_require__("src/first_empty_quasis/index.js")`,
`'./first_empty_quasis/': ()=>__mako_require__("src/first_empty_quasis/index.js")`,
`'./first_empty_quasis/index': ()=>__mako_require__("src/first_empty_quasis/index.js")`,
`'./first_empty_quasis/index.js': ()=>__mako_require__("src/first_empty_quasis/index.js")`,
`'./first_empty_quasis/other': ()=>__mako_require__("src/first_empty_quasis/other.js")`,
`'./first_empty_quasis/other.js': ()=>__mako_require__("src/first_empty_quasis/other.js")`,
`'./i18n/en-US': ()=>__mako_require__("src/i18n/en-US.json")`,
`'./i18n/en-US.json': ()=>__mako_require__("src/i18n/en-US.json")`,
`'./i18n/zh-CN': ()=>__mako_require__("src/i18n/zh-CN.json")`,
`'./i18n/zh-CN.json': ()=>__mako_require__("src/i18n/zh-CN.json")`,
`'./index': ()=>__mako_require__("src/index.ts")`,
`'./index.ts': ()=>__mako_require__("src/index.ts")`
].join(',\n\\s+'),
true,
),
Expand All @@ -42,14 +46,14 @@ assert.match(
moduleReg(
"src/fake.js\\?context&glob=\\*\\*/\\*",
[
"'./a.js': ()=>__mako_require__(\"src/fake.js/a.js\")",
"'.': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./a': ()=>__mako_require__(\"src/fake.js/a.js\")",
"'./aa.js': ()=>__mako_require__(\"src/fake.js/aa.js\")",
"'./a.js': ()=>__mako_require__(\"src/fake.js/a.js\")",
"'./aa': ()=>__mako_require__(\"src/fake.js/aa.js\")",
"'./index.js': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./aa.js': ()=>__mako_require__(\"src/fake.js/aa.js\")",
"'./index': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'.': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./': ()=>__mako_require__(\"src/fake.js/index.js\")",
"'./index.js': ()=>__mako_require__(\"src/fake.js/index.js\")",
].join(',\n\\s+'),
true,
),
Expand Down Expand Up @@ -125,3 +129,5 @@ assert.doesNotMatch(
/glob=\*\*\/\*\s*\*\//,
"should escape glob pattern in module id debug comment"
);

assert.match(content, /src\/first_empty_quasis\?context&glob=\*\*\/\*&async/g, "should handle template string with first empty quasis");
3 changes: 2 additions & 1 deletion ...vascript.require-dynamic/mako.config.json → ...avascript.context-module/mako.config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
"alias": [
["@", "./src"]
]
}
},
"progress":false
}
0 ...avascript.require-dynamic/src/ext/json.ts → ...javascript.context-module/src/ext/json.ts
View file
Open in desktop
File renamed without changes.
0 ...vascript.require-dynamic/src/fake.js/a.js → ...avascript.context-module/src/fake.js/a.js
View file
Open in desktop
File renamed without changes.
0 ...ascript.require-dynamic/src/fake.js/aa.js → ...vascript.context-module/src/fake.js/aa.js
View file
Open in desktop
File renamed without changes.
0 ...ript.require-dynamic/src/fake.js/index.js → ...cript.context-module/src/fake.js/index.js
View file
Open in desktop
File renamed without changes.
3 changes: 3 additions & 0 deletions e2e/fixtures/javascript.context-module/src/first_empty_quasis/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function loadUrl(url) {
return import(`${url}`)
}
Comment on lines +1 to +3
Copy link

@coderabbitai coderabbitai bot Dec 11, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议增加错误处理和输入验证

当前实现过于简单，建议添加以下改进：

  1. 添加参数验证
  2. 添加错误处理
  3. 添加函数文档说明用途和参数要求

建议按照以下方式重构：

+/**
+ * 动态加载URL指定的模块
+ * @param {string} url - 要加载的模块URL
+ * @returns {Promise<any>} 返回加载的模块
+ * @throws {Error} 当URL无效或模块加载失败时抛出错误
+ */
 export function loadUrl(url) {
+  if (typeof url !== 'string' || !url.trim()) {
+    throw new Error('URL must be a non-empty string');
+  }
+
+  try {
     return import(`${url}`)
+  } catch (error) {
+    throw new Error(`Failed to load module at ${url}: ${error.message}`);
+  }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function loadUrl(url) {
return import(`${url}`)
}
/**
* 动态加载URL指定的模块
* @param {string} url - 要加载的模块URL
* @returns {Promise<any>} 返回加载的模块
* @throws {Error} 当URL无效或模块加载失败时抛出错误
*/
export function loadUrl(url) {
if (typeof url !== 'string' || !url.trim()) {
throw new Error('URL must be a non-empty string');
}
try {
return import(`${url}`)
} catch (error) {
throw new Error(`Failed to load module at ${url}: ${error.message}`);
}
}

1 change: 1 addition & 0 deletions e2e/fixtures/javascript.context-module/src/first_empty_quasis/other.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const other = "other"
0 ...cript.require-dynamic/src/i18n/en-US.json → ...script.context-module/src/i18n/en-US.json
View file
Open in desktop
File renamed without changes.
0 ...cript.require-dynamic/src/i18n/zh-CN.json → ...script.context-module/src/i18n/zh-CN.json
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions ...s/javascript.require-dynamic/src/index.ts → ...es/javascript.context-module/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,4 @@ console.log(loadFile('/zh-CN.json'));
console.log(loadFile2('/a.js'));
console.log(loadFile3('a.js'));

require("./first_empty_quasis")
Loading