summaryrefslogtreecommitdiff
path: root/cli/util
diff options
context:
space:
mode:
authorYusuke Tanaka <yusuktan@maguro.dev>2024-09-19 16:19:40 +0900
committerGitHub <noreply@github.com>2024-09-19 00:19:40 -0700
commit0ea71abdefceb156e28f90bdfb7ca7a9914ec0c8 (patch)
treeaf2932b95322f46783cd47cdab691443291ec610 /cli/util
parent486cb18fc54b408d40a02415e48e0cfb6cb196ed (diff)
fix(cli): handle edge cases around `export`s in doc tests and default export (#25720)
This commit fixes issues with the pseudo test file generation logic, namely: - `export`s declared in snippets - auto import insertion for `default export` ## Case 1: `export`s declared in snippets In the previous implementation, `export`s declared in snippets were moved to the top level of the module in the generated pseudo test file. This is required because `export` must be at the top level. This becomes a problem if such a `export` has a body, containing a reference to a local variable. Suppose we extract this snippet from JSDoc: ```ts const logger = createLogger("my-awesome-module"); export function sum(a: number, b: number): number { logger.debug("sum called"); return a + b; } ``` This gets converted into the following invalid code (note that `export function sum` is moved to the top level, but its body references `logger` variable which can't be referenced from here): ```ts export function sum(a: number, b: number): number { logger.debug("sum called"); return a + b; } Deno.test("./base.ts$1-7.ts", async () => { const logger = createLogger("my-awesome-module"); }); ``` To resolve this issue, this commit adds a logic to remove the `export` keyword, allowing the exported items to stay in the `Deno.test` block scope, like so: ```ts Deno.test("./base.ts$1-7.ts", async () => { const logger = createLogger("my-awesome-module"); function sum(a: number, b: number): number { logger.debug("sum called"); return a + b; } }); ``` ## Case 2: default export Previously `default export foo` was not captured by the export collector, so auto import insertion didn't work for this case. To put it concretely, the following code snippet didn't work when run with `deno test --doc` because `import foo from "file:///path/to/mod.ts"` didn't get inserted automatically: ```ts /** * ```ts * console.log(foo); * ``` * * @module */ const foo = 42; export default foo; ``` This commit fixes this issue and the above example works fine. --- Fixes #25718
Diffstat (limited to 'cli/util')
-rw-r--r--cli/util/extract.rs246
1 files changed, 214 insertions, 32 deletions
diff --git a/cli/util/extract.rs b/cli/util/extract.rs
index e27a79347..841cf6eb0 100644
--- a/cli/util/extract.rs
+++ b/cli/util/extract.rs
@@ -358,7 +358,18 @@ impl Visit for ExportCollector {
self.default_export = Some(ident.sym.clone());
}
}
- ast::DefaultDecl::TsInterfaceDecl(_) => {}
+ ast::DefaultDecl::TsInterfaceDecl(iface_decl) => {
+ self.default_export = Some(iface_decl.id.sym.clone());
+ }
+ }
+ }
+
+ fn visit_export_default_expr(
+ &mut self,
+ export_default_expr: &ast::ExportDefaultExpr,
+ ) {
+ if let ast::Expr::Ident(ident) = &*export_default_expr.expr {
+ self.default_export = Some(ident.sym.clone());
}
}
@@ -472,7 +483,7 @@ fn extract_sym_from_pat(pat: &ast::Pat) -> Vec<Atom> {
/// });
/// ```
///
-/// # Edge case - duplicate identifier
+/// # Edge case 1 - duplicate identifier
///
/// If a given file imports, say, `doSomething` from an external module while
/// the base file exports `doSomething` as well, the generated pseudo test file
@@ -491,6 +502,52 @@ fn extract_sym_from_pat(pat: &ast::Pat) -> Vec<Atom> {
/// assertEquals(doSomething(1), 2);
/// });
/// ```
+///
+/// # Edge case 2 - exports can't be put inside `Deno.test` blocks
+///
+/// All exports like `export const foo = 42` must be at the top level of the
+/// module, making it impossible to wrap exports in `Deno.test` blocks. For
+/// example, when the following code snippet is provided:
+///
+/// ```ts
+/// const logger = createLogger("my-awesome-module");
+///
+/// export function sum(a: number, b: number): number {
+/// logger.debug("sum called");
+/// return a + b;
+/// }
+/// ```
+///
+/// If we applied the naive transformation to this, the generated pseudo test
+/// file would look like:
+///
+/// ```ts
+/// Deno.test("./base.ts$1-7.ts", async () => {
+/// const logger = createLogger("my-awesome-module");
+///
+/// export function sum(a: number, b: number): number {
+/// logger.debug("sum called");
+/// return a + b;
+/// }
+/// });
+/// ```
+///
+/// But obviously this violates the rule because `export function sum` is not
+/// at the top level of the module.
+///
+/// To address this issue, the `export` keyword is removed so that the item can
+/// stay in the `Deno.test` block's scope:
+///
+/// ```ts
+/// Deno.test("./base.ts$1-7.ts", async () => {
+/// const logger = createLogger("my-awesome-module");
+///
+/// function sum(a: number, b: number): number {
+/// logger.debug("sum called");
+/// return a + b;
+/// }
+/// });
+/// ```
fn generate_pseudo_file(
file: File,
base_file_specifier: &ModuleSpecifier,
@@ -553,9 +610,51 @@ impl<'a> VisitMut for Transform<'a> {
for item in &module.body {
match item {
- ast::ModuleItem::ModuleDecl(decl) => {
- module_decls.push(decl.clone());
- }
+ ast::ModuleItem::ModuleDecl(decl) => match self.wrap_kind {
+ WrapKind::NoWrap => {
+ module_decls.push(decl.clone());
+ }
+ // We remove `export` keywords so that they can be put inside
+ // `Deno.test` block scope.
+ WrapKind::DenoTest => match decl {
+ ast::ModuleDecl::ExportDecl(export_decl) => {
+ stmts.push(ast::Stmt::Decl(export_decl.decl.clone()));
+ }
+ ast::ModuleDecl::ExportDefaultDecl(export_default_decl) => {
+ let stmt = match &export_default_decl.decl {
+ ast::DefaultDecl::Class(class) => {
+ let expr = ast::Expr::Class(class.clone());
+ ast::Stmt::Expr(ast::ExprStmt {
+ span: DUMMY_SP,
+ expr: Box::new(expr),
+ })
+ }
+ ast::DefaultDecl::Fn(func) => {
+ let expr = ast::Expr::Fn(func.clone());
+ ast::Stmt::Expr(ast::ExprStmt {
+ span: DUMMY_SP,
+ expr: Box::new(expr),
+ })
+ }
+ ast::DefaultDecl::TsInterfaceDecl(ts_interface_decl) => {
+ ast::Stmt::Decl(ast::Decl::TsInterface(
+ ts_interface_decl.clone(),
+ ))
+ }
+ };
+ stmts.push(stmt);
+ }
+ ast::ModuleDecl::ExportDefaultExpr(export_default_expr) => {
+ stmts.push(ast::Stmt::Expr(ast::ExprStmt {
+ span: DUMMY_SP,
+ expr: export_default_expr.expr.clone(),
+ }));
+ }
+ _ => {
+ module_decls.push(decl.clone());
+ }
+ },
+ },
ast::ModuleItem::Stmt(stmt) => {
stmts.push(stmt.clone());
}
@@ -880,6 +979,32 @@ Deno.test("file:///main.ts$13-16.ts", async ()=>{
},
],
},
+ Test {
+ input: Input {
+ source: r#"
+/**
+ * ```ts
+ * foo();
+ * ```
+ */
+export function foo() {}
+
+export const ONE = 1;
+const TWO = 2;
+export default TWO;
+"#,
+ specifier: "file:///main.ts",
+ },
+ expected: vec![Expected {
+ source: r#"import TWO, { ONE, foo } from "file:///main.ts";
+Deno.test("file:///main.ts$3-6.ts", async ()=>{
+ foo();
+});
+"#,
+ specifier: "file:///main.ts$3-6.ts",
+ media_type: MediaType::TypeScript,
+ }],
+ },
// Avoid duplicate imports
Test {
input: Input {
@@ -945,30 +1070,43 @@ Deno.test("file:///main.ts$3-7.ts", async ()=>{
media_type: MediaType::TypeScript,
}],
},
- // example code has an exported item `foo` - because `export` must be at
- // the top level, `foo` is "hoisted" to the top level instead of being
- // wrapped in `Deno.test`.
+ // https://github.com/denoland/deno/issues/25718
+ // A case where the example code has an exported item which references
+ // a variable from one upper scope.
+ // Naive application of `Deno.test` wrap would cause a reference error
+ // because the variable would go inside the `Deno.test` block while the
+ // exported item would be moved to the top level. To suppress the auto
+ // move of the exported item to the top level, the `export` keyword is
+ // removed so that the item stays in the same scope as the variable.
Test {
input: Input {
source: r#"
/**
* ```ts
- * doSomething();
- * export const foo = 42;
+ * import { getLogger } from "@std/log";
+ *
+ * const logger = getLogger("my-awesome-module");
+ *
+ * export function foo() {
+ * logger.debug("hello");
+ * }
* ```
+ *
+ * @module
*/
-export function doSomething() {}
"#,
specifier: "file:///main.ts",
},
expected: vec![Expected {
- source: r#"export const foo = 42;
-import { doSomething } from "file:///main.ts";
-Deno.test("file:///main.ts$3-7.ts", async ()=>{
- doSomething();
+ source: r#"import { getLogger } from "@std/log";
+Deno.test("file:///main.ts$3-12.ts", async ()=>{
+ const logger = getLogger("my-awesome-module");
+ function foo() {
+ logger.debug("hello");
+ }
});
"#,
- specifier: "file:///main.ts$3-7.ts",
+ specifier: "file:///main.ts$3-12.ts",
media_type: MediaType::TypeScript,
}],
},
@@ -1103,26 +1241,23 @@ assertEquals(add(1, 2), 3);
media_type: MediaType::TypeScript,
}],
},
- // duplication of imported identifier and local identifier is fine, since
- // we wrap the snippet in a block.
- // This would be a problem if the local one is declared with `var`, as
- // `var` is not block scoped but function scoped. For now we don't handle
- // this case assuming that `var` is not used in modern code.
+ // If the snippet has a local variable with the same name as an exported
+ // item, the local variable takes precedence.
Test {
input: Input {
source: r#"
- /**
- * ```ts
- * const foo = createFoo();
- * foo();
- * ```
- */
- export function createFoo() {
- return () => "created foo";
- }
+/**
+ * ```ts
+ * const foo = createFoo();
+ * foo();
+ * ```
+ */
+export function createFoo() {
+ return () => "created foo";
+}
- export const foo = () => "foo";
- "#,
+export const foo = () => "foo";
+"#,
specifier: "file:///main.ts",
},
expected: vec![Expected {
@@ -1134,6 +1269,38 @@ foo();
media_type: MediaType::TypeScript,
}],
},
+ // Unlike `extract_doc_tests`, `extract_snippet_files` does not remove
+ // the `export` keyword from the exported items.
+ Test {
+ input: Input {
+ source: r#"
+/**
+ * ```ts
+ * import { getLogger } from "@std/log";
+ *
+ * const logger = getLogger("my-awesome-module");
+ *
+ * export function foo() {
+ * logger.debug("hello");
+ * }
+ * ```
+ *
+ * @module
+ */
+"#,
+ specifier: "file:///main.ts",
+ },
+ expected: vec![Expected {
+ source: r#"import { getLogger } from "@std/log";
+export function foo() {
+ logger.debug("hello");
+}
+const logger = getLogger("my-awesome-module");
+"#,
+ specifier: "file:///main.ts$3-12.ts",
+ media_type: MediaType::TypeScript,
+ }],
+ },
Test {
input: Input {
source: r#"
@@ -1312,6 +1479,21 @@ assertEquals(add(1, 2), 3);
default_expected: Some("foo".into()),
},
Test {
+ input: r#"export default class Foo {}"#,
+ named_expected: atom_set!(),
+ default_expected: Some("Foo".into()),
+ },
+ Test {
+ input: r#"export default interface Foo {}"#,
+ named_expected: atom_set!(),
+ default_expected: Some("Foo".into()),
+ },
+ Test {
+ input: r#"const foo = 42; export default foo;"#,
+ named_expected: atom_set!(),
+ default_expected: Some("foo".into()),
+ },
+ Test {
input: r#"export { foo, bar as barAlias };"#,
named_expected: atom_set!("foo", "barAlias"),
default_expected: None,