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.

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(template-compiler): fix static listeners with deep paths #3807

Merged
merged 11 commits into from
Oct 17, 2023

Conversation

nolanlawson
Copy link
Contributor

Details

Fixes #3804

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

@nolanlawson nolanlawson requested a review from a team as a code owner October 16, 2023 21:55

// Cloning here is necessary because `this.replace()` is destructive, and we might use the
// node later during static content optimization
expression = doStructuredClone(expression);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a really subtle bug – the code below is mutating the AST, which only becomes a problem later if we try to re-use it, which we do during the static content optimization, which is a second pass on top of the first pass we already did.

First pass:

data.push(codeGen.genEventListeners(listeners));

Second pass:

addDatabagProp(this.genEventListeners(node.listeners));

const doStructuredClone =
typeof structuredClone === 'function'
? structuredClone
: (obj: any) => JSON.parse(JSON.stringify(obj));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manually ran the Karma tests in Node 16 just to be safe.

@nolanlawson
Copy link
Contributor Author

Looks like the AST format changed... will need another look.

@nolanlawson
Copy link
Contributor Author

Found a pre-existing issue: #3808

nolanlawson and others added 4 commits October 17, 2023 08:42
…ener/deepListener.html

Co-authored-by: Eugene Kashida <ekashida@gmail.com>
…ener/deepListener.js

Co-authored-by: Eugene Kashida <ekashida@gmail.com>
…ener/deepListener.js

Co-authored-by: Eugene Kashida <ekashida@gmail.com>
…ener/deepListener.js

Co-authored-by: Eugene Kashida <ekashida@gmail.com>
Copy link
Member

@ekashida ekashida left a comment

Choose a reason for hiding this comment

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

lgtm, let's wait to see whether the ast changes are bueno

@nolanlawson nolanlawson merged commit 5bf11b9 into master Oct 17, 2023
11 checks passed
@nolanlawson nolanlawson deleted the nolan/deep-listener branch October 17, 2023 19:22
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.

[Template compiler] Event listeners on deep objects not being serialized properly
3 participants