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

Ensure cloning of incomming data #891

Merged
merged 3 commits into from
Oct 24, 2017
Merged

Ensure cloning of incomming data #891

merged 3 commits into from
Oct 24, 2017

Conversation

petterek
Copy link

Please verify syntax

Please verify syntax
@Conduitry
Copy link
Member

@petterek
Copy link
Author

Yes they are :) hopefully..
I think cloning of incomming data makes sense, to be able to detect chagnes, in case of date changing within the data itself, and then telling the component to update itself

@Conduitry
Copy link
Member

Conduitry commented Oct 15, 2017

Oof, making updates to branches on forks is not the most ergonomic experience, but at least I know how to do it now :)

Fixed the syntax, and updated the tests. Thanks for the suggestion @petterek

@Rich-Harris
Copy link
Member

Thanks for the PR @petterek. I read the Gitter convo but I'm not entirely clear on what bug this is fixing? I ask because it would probably be preferable not to call assign for every component (since that includes nested components, which could number in the thousands) unless there's no other solution.

@petterek
Copy link
Author

It might be i'm missusing svelte.. :) but..
If you change on of the fields of your data directly, without using component.set() and then try to use the set method to "rerender" the view. Svelte will not "detect" the change the first time, but.. the next time it wil.
The issue is only "visible" if you have no data() on the component.. So the cloning is already there but not if the data() is null.

I can fix my issue by letting the data() of the component return an empty object.

@Rich-Harris
Copy link
Member

That's odd... can you create a repro?

@Conduitry
Copy link
Member

@Rich-Harris I am actually able to reproduce what I think @petterek is referring to - https://gist.github.com/Conduitry/22dc70fcca81f5836989bd750c35baa0 - The app continues to render '0' and not '42'. If I include a <script> export default { data() { return {} } } </script> in the component, then this works correctly.

@Rich-Harris Rich-Harris merged commit 2800b5c into sveltejs:master Oct 24, 2017
Rich-Harris added a commit that referenced this pull request Oct 24, 2017
@Rich-Harris
Copy link
Member

Ah, got it, thanks. Understand the problem now. Definitely a bug that needed to be fixed — I think at some point we might want to have a slightly different initialisation process for nested versus root-level components, so that we can assign where we need to and skip it where we don't, plus any other differences that exist between them (e.g. the order of operations is slightly different viz rendering). Will park that for now though.

Thanks for the fix @petterek 👍

@Rich-Harris
Copy link
Member

PS I added a test based on @Conduitry's gist

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