Skip to content

Feature: Fix snippet dependency resolution for JS output#1158

Open
maptoan wants to merge 1 commit intosource-academy:masterfrom
maptoan:contribai/feat/fix-snippet-dependency-resolution-for-js
Open

Feature: Fix snippet dependency resolution for JS output#1158
maptoan wants to merge 1 commit intosource-academy:masterfrom
maptoan:contribai/feat/fix-snippet-dependency-resolution-for-js

Conversation

@maptoan
Copy link
Copy Markdown

@maptoan maptoan commented Mar 30, 2026

Problem

The processSnippetJs function currently does not correctly use the snippetStore to prepend the code of required snippets. This prevents programs that rely on previously defined sets from running.

Severity: medium
File: javascript/processingFunctions/processSnippetJs.js

Solution

Update processSnippetJs to use recursiveGetRequires to collect all required code strings from the snippetStore and prepend them to the main snippet code.

Changes

  • javascript/processingFunctions/processSnippetJs.js (modified)

Testing

  • Existing tests pass
  • Manual review completed
  • No new warnings/errors introduced

Closes #840

The `processSnippetJs` function currently does not correctly use the `snippetStore` to prepend the code of required snippets. This prevents programs that rely on previously defined sets from running.

Affected files: processSnippetJs.js

Signed-off-by: toanmap <174589430+maptoan@users.noreply.github.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the requirement and example processing logic in processSnippetJs.js to use a unified Set for dependency collection. However, the changes introduce a SyntaxError because the examples constant is declared twice. Feedback also includes suggestions to adopt idiomatic for...of loops for better readability and to restore the trailing newline at the end of the file for consistency with standard practices.

for (let i = 0; requirements[i]; i++) {
recursiveGetRequires(requirements[i].firstChild.nodeValue, reqSet);
}
const examples = node.getElementsByTagName("EXAMPLE");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The const variable examples is declared here, but it is re-declared on line 115. This will cause a SyntaxError: Identifier 'examples' has already been declared'. Please declare this variable only once and reuse it for both loops that iterate over the examples.

Comment on lines +95 to 103
const requirements = node.getElementsByTagName("REQUIRES");
for (let i = 0; requirements[i]; i++) {
recursiveGetRequires(requirements[i].firstChild.nodeValue, reqSet);
}
const examples = node.getElementsByTagName("EXAMPLE");
for (let i = 0; examples[i]; i++) {
const exampleName = examples[i].firstChild.nodeValue;
recursiveGetRequires(exampleName, reqSet);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

These C-style loops can be simplified by using for...of loops, which are more idiomatic in modern JavaScript and improve readability.

    const requirements = node.getElementsByTagName("REQUIRES");
    for (const requirement of requirements) {
      recursiveGetRequires(requirement.firstChild.nodeValue, reqSet);
    }
    const examples = node.getElementsByTagName("EXAMPLE");
    for (const example of examples) {
      recursiveGetRequires(example.firstChild.nodeValue, reqSet);
    }

};

export default processSnippetJs;
export default processSnippetJs; No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

It's a common convention and a good practice to have a newline at the end of a file. Many tools and version control systems expect it. Please consider adding it back.

export default processSnippetJs;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WEB: 2.3.3 some programs not runnable

1 participant