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(ssr): normalize class in child attrs #4953

Merged
merged 6 commits into from
Nov 26, 2024

Conversation

cardoso
Copy link
Contributor

@cardoso cardoso commented Nov 26, 2024

Details

There might be a better way to do this, but it could be enough for now.

I updated the tests added in #4931 to cover this case. It still doesn't fix any expected failures but reduces the diff further:

attribute-class/unstyled/dynamic/index.js (15 to 10)
attribute-class/with-scoped-styles-only-in-child/dynamic/index.js (25 to 20)
attribute-class/with-scoped-styles-only-in-parent/dynamic/index.js (15 to 10)
attribute-class/with-scoped-styles/dynamic/index.js (25 to 20)

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🔬 Yes, it does include an observable change.

Template class binding should mostly work for child components now.

GUS work item

@cardoso cardoso requested a review from a team as a code owner November 26, 2024 13:52
Copy link
Contributor

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

Looking great so far, just a small issue with ctx.import() and a request for a new test. Thanks a bunch! 🥇

<x-child class="button__icon">
<template shadowrootmode="open">
</template>
</x-child>
Copy link
Contributor

Choose a reason for hiding this comment

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

Another case we should cover here for safety: dynamic components (lwc:component). These technically go down a different code path (LwcComponent.ts), although you should still hit it since it will call into getChildAttrsOrProps. As mentioned elsewhere though, I think this should be a separate standalone fixture.

Copy link
Contributor Author

@cardoso cardoso Nov 26, 2024

Choose a reason for hiding this comment

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

I hit an interesting behavior when first doing a single fixture:

If I imported 'x/child' and passed its constructor into lwc:component while also using it directly as <x-child></x-child>.

__SYMBOL__GENERATE_MARKUP is not defined

Is this issue known? Doesn't seem convered by the existing fixtures.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like an unrelated error!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #4958

@cardoso cardoso requested a review from nolanlawson November 26, 2024 18:42
Copy link
Contributor

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks 🌞

@nolanlawson
Copy link
Contributor

/nucleus test

@nolanlawson nolanlawson merged commit dbbc15c into salesforce:master Nov 26, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants