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: revert "prevent prematurely cloning light dom slot vnodes" #4454

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

nolanlawson
Copy link
Collaborator

@nolanlawson nolanlawson commented Aug 9, 2024

Details

This reverts commit 1105823 (#4258).

The fix for #4446 is ready to go (#4452), but for 252 (Winter '25) I think it would be pragmatic to just revert #4258 entirely.

This will make the 252 behavior the same as 250. I think this is the right choice, because:

  1. fix: fix light dom slot forwarding bug #4452 is quite complex, and we probably want time to bake in main
  2. The bug introduced by fix: prevent prematurely cloning light dom slot vnodes #4258 is IMO more severe than the bug it was fixing (not rendering DOM nodes at all is more severe than not calling disconnectedCallback – synthetic lifecycle already has plenty of bugs where it doesn't call disconnectedCallback, which mostly manifests as memory leaks rather than DOM nodes failing to render).

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.

Yes, 252 behavior will go back to 250 behavior (disconnectedCallback not firing correctly in certain tricky cases with light DOM slot forwarding).

@nolanlawson nolanlawson requested a review from a team as a code owner August 9, 2024 18:58
Copy link
Member

@jmsjtu jmsjtu left a comment

Choose a reason for hiding this comment

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

I agree with this approach, the number of impacted customers was very low prior to this fix too.

Copy link
Contributor

❌ One or More Pre-release Checks Failed

  • Check PR user has rights to execute the workflow
    PR owner is allowed to initiate release/deploy

  • Check PR is approved
    PR 4454 is approved

  • Check PR is mergeable
    PR 4454 is mergeable

  • ⚠️ Check GUS reference is present
    PR does not contain a reference to a GUS work item. You can update it any time by including '@W-XXXXXXX' into the title or body of the PR

  • Check no conflicting Github release
    A Github release already exists with a tag of v7.1.2. Please commit/push a version bump to the nolan/revert-light-dom-slot branch or have Nucleus do it for you by responding with the following comment
    /nucleus bump-version [version]

    To skip this check, run /nucleus release --force

  • Check NPM packages have not been released yet
    Version 7.1.2 of this project has already been published. If you would like to skip NPM publishing, then run /nucleus release --skipNpmRelease.If you want to bump the version in your source branch, run /nucleus bump-version [version]

  • ⚪️ Check Maven artifacts have not been released yet (skipped)
    Skipping check since project doesn't deliver Maven artifacts

  • ⚪️ Check GAV mapping exists for jar repo (skipped)
    Skipping GAV mapping check because a core jar is not being released.

Copy link
Contributor

❌ One or More Pre-release Checks Failed

  • Check PR user has rights to execute the workflow
    PR owner is allowed to initiate release/deploy

  • Check PR is approved
    PR 4454 is approved

  • Check PR is mergeable
    PR 4454 is mergeable

  • ⚠️ Check GUS reference is present
    PR does not contain a reference to a GUS work item. You can update it any time by including '@W-XXXXXXX' into the title or body of the PR

  • Check no conflicting Github release
    A Github release already exists with a tag of v7.1.2. Please commit/push a version bump to the nolan/revert-light-dom-slot branch or have Nucleus do it for you by responding with the following comment
    /nucleus bump-version [version]

    To skip this check, run /nucleus release --force

  • Check NPM packages have not been released yet
    Version 7.1.2 of this project has already been published. If you would like to skip NPM publishing, then run /nucleus release --skipNpmRelease.If you want to bump the version in your source branch, run /nucleus bump-version [version]

  • ⚪️ Check Maven artifacts have not been released yet (skipped)
    Skipping check since project doesn't deliver Maven artifacts

  • ⚪️ Check GAV mapping exists for jar repo (skipped)
    Skipping GAV mapping check because a core jar is not being released.

@ekashida ekashida merged commit f07fbd0 into winter25 Aug 13, 2024
11 of 12 checks passed
@ekashida ekashida deleted the nolan/revert-light-dom-slot branch August 13, 2024 18:26
Copy link
Contributor

❌ One or More Pre-release Checks Failed

  • ⚪️ Check PR user has rights to execute the workflow (skipped)
    Skipping PR owner check since PR has already been merged.

  • Check PR is approved
    PR 4454 is approved

  • ⚪️ Check PR is mergeable (skipped)
    PR 4454 has already been merged

  • ⚠️ Check GUS reference is present
    PR does not contain a reference to a GUS work item. You can update it any time by including '@W-XXXXXXX' into the title or body of the PR

  • Check no conflicting Github release
    A Github release already exists with a tag of v7.1.2. Please commit/push a version bump to the nolan/revert-light-dom-slot branch or have Nucleus do it for you by responding with the following comment
    /nucleus bump-version [version]

    To skip this check, run /nucleus release --force

  • Check NPM packages have not been released yet
    Version 7.1.2 of this project has already been published. If you would like to skip NPM publishing, then run /nucleus release --skipNpmRelease.If you want to bump the version in your source branch, run /nucleus bump-version [version]

  • ⚪️ Check Maven artifacts have not been released yet (skipped)
    Skipping check since project doesn't deliver Maven artifacts

  • ⚪️ Check GAV mapping exists for jar repo (skipped)
    Skipping GAV mapping check because a core jar is not being released.

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.

3 participants