Skip to content
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

fix(cli): fix stack overflow in Deno doc when namespace exports itself #599

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(cli): fix stack overflow in Deno doc when namespace exports itself #599

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions src/parser.rs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

We should add a test for this in tests/specs (copy and paste another .txt file, modify it, then run cargo test to see if it works. You can use UPDATE=1 env var to update the expected output with the actual output)

Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,10 @@ impl<'a> DocParser<'a> {
let mut handled_symbols = HashSet::new();

for (export_name, export_symbol_id) in symbol.exports() {
if export_name == &namespace_name
{
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

What happens for the following code?

export declare namespace test {
    const test: string;
    export { test };
}

Or the following:

export declare namespace test {
    export class test {}
}

I think it can't be based on strings, but probably needs to be based on the symbol id or swc id.

Copy link
Contributor Author

@MujahedSafaa MujahedSafaa Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I previously attempted to use the symbol ID, but the output varied for certain inputs. For instance:

export function describe(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>;
export function describe(name?: string, fn?: SuiteFn): Promise<void>;
export function describe(options?: TestOptions, fn?: SuiteFn): Promise<void>;
export function describe(fn?: SuiteFn): Promise<void>;

export namespace describe {
    /**
     * Shorthand for skipping a suite, equivalent to `describe([name], { skip: true }[, fn])`.
     */
    export function skip(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function skip(name?: string, fn?: SuiteFn): Promise<void>;
    export function skip(options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function skip(fn?: SuiteFn): Promise<void>;

    /**
     * Shorthand for marking a suite as `TODO`, equivalent to `describe([name], { todo: true }[, fn])`.
     */
    export function todo(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function todo(name?: string, fn?: SuiteFn): Promise<void>;
    export function todo(options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function todo(fn?: SuiteFn): Promise<void>;

    /**
     * Shorthand for marking a suite as `only`, equivalent to `describe([name], { only: true }[, fn])`.
     * @since v18.15.0
     */
    export function only(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function only(name?: string, fn?: SuiteFn): Promise<void>;
    export function only(options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function only(fn?: SuiteFn): Promise<void>;
}

The documentation is only generated for the first line: export function describe(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>; The rest are skipped because they share the same symbol ID. I guess the symbol ID is same since they all have the same symbol and module id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @dsherret
I wanted to follow up on my previous comment. Could you please check my comment above when you get a chance?

handled_symbols.insert(UniqueSymbolId::new(
module_info.module_id(),
*export_symbol_id,
Expand Down