Skip to content

Conversation

eps1lon
Copy link

@eps1lon eps1lon commented Apr 29, 2025

Fixes TypeError: Cannot read properties of null (reading 'firstChild') if a <template> was the last element.

Existing tests didn't catch this since <template> was followed by some whitespace. However, when we manually advance the node, we need to check the enter condition of the loop again since nextNode does not accept a nullable node.

@CLAassistant
Copy link

CLAassistant commented Apr 29, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a crash in the toolbox optimizer that occurs when a <template> element is the very last element in the DOM, preventing a TypeError when trying to read properties of null.

  • Adds a null check after advancing the node when skipping template elements
  • Includes regression test with template as the last element without trailing whitespace

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/optimizer/lib/transformers/OptimizeAmpBind.js Adds null check to prevent crash when template is last element
packages/optimizer/spec/transformers/valid/OptimizeAmpBind/template_last/input.html Test case with template as final element without whitespace
packages/optimizer/spec/transformers/valid/OptimizeAmpBind/template_last/expected_output.html Expected output for the regression test

Comment on lines +48 to +50
} else {
continue;
}
Copy link
Preview

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

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

The else block containing only 'continue' is unnecessary. The continue statement can be moved outside the if-else block since it should execute in both cases when node is not null.

Suggested change
} else {
continue;
}
}
continue;

Copilot uses AI. Check for mistakes.

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.

2 participants