Skip to content

Commit

Permalink
fix(cli): handle edge cases around exports in doc tests and default…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
@magurotuna
magurotuna committed Sep 19, 2024
1 parent 486cb18 commit 0ea71ab
Showing 1 changed file with 214 additions and 32 deletions.
246 changes: 214 additions & 32 deletions cli/util/extract.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
}],
},
Expand Down Expand Up @@ -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 {
Expand All @@ -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#"
Expand Down Expand Up @@ -1311,6 +1478,21 @@ assertEquals(add(1, 2), 3);
named_expected: atom_set!(),
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"),
Expand Down

0 comments on commit 0ea71ab

Please sign in to comment.