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: svelte:component evaluates props once #8946

Merged

Conversation

ngtr6788
Copy link
Contributor

Fixes #6634

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

@ngtr6788 ngtr6788 marked this pull request as ready for review July 10, 2023 01:35
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thank you! Could you add a changeset? Then we're good to merge.

@dummdidumm dummdidumm merged commit 66593c6 into sveltejs:master Jul 10, 2023
6 of 7 checks passed
@github-actions github-actions bot mentioned this pull request Jul 10, 2023
@dorisip
Copy link

dorisip commented Jul 19, 2023

I think this change results in reactive props not being picked up by the component (See here).

Is this the new expected behavior?

@hackape
Copy link
Contributor

hackape commented Jul 20, 2023

image

@dorisip I looked into your repro and noticed a weird thing, switch_props(ctx) should have ctx as arg but is missing now. But I it isn't introduced by this PR, cus I also looked into v4.0.0 compiled code and the ctx is already missing there. Good catch through!

@jkbz64
Copy link

jkbz64 commented Jul 20, 2023

I'm having the same issue, this breaks my project, reactive props { ...props } do not change for components in svelte:component.

@hackape
Copy link
Contributor

hackape commented Jul 20, 2023

My investigation shows that, the bug actually originates from the implementation of switch_props(ctx) function, because ctx param is not used anywhere in the function body. Although compiled js code from Svelte compiler retain the ctx arg, I suspect it's optimized away by rollup bundler in REPL.

Current impl of switch_props fails to update switch_instance_props correctly, because it relies on outdated switch_instance_spread_levels, thus results in the bug.

const switch_instance_spread_levels = [/*props*/ ctx[1]];
var switch_value = /*view*/ ctx[0];

function switch_props(ctx) {
    let switch_instance_props = {};
    
    for (let i = 0; i < switch_instance_spread_levels.length; i += 1) {
	    switch_instance_props = assign(switch_instance_props, switch_instance_spread_levels[i]);
    }
    
    return {
	    props: switch_instance_props,
	    $$inline: true
    };
}

I tried to backtrace history of switch_props and couldn't find where it began to drop reference to ctx, it's been wrong for a really long time, 4+ years at least. This PR's is correct, it resolves a bug, it was that bug which shadows the real problem.

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.

<svelte:component> evaluates props twice
5 participants