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

Computed properties break _rerender #6

Closed
aphitiel opened this issue Mar 15, 2018 · 5 comments
Closed

Computed properties break _rerender #6

aphitiel opened this issue Mar 15, 2018 · 5 comments

Comments

@aphitiel
Copy link
Contributor

When a component uses computed properties, _rerender tries to set() those, which makes _checkReadOnly inside the component throw. This leads to a full reload when using svelte-loader with hotReload.

I couldn't find way to tell if a property is computed, though a possible workaround seems to be replacing this line with

  this.proxyTarget._updatingReadonlyProperty = true;
  this.set(oldstate);
  this.proxyTarget._updatingReadonlyProperty = false;

Although that feels somewhat dirty. What do you think?

@ekhaled
Copy link
Owner

ekhaled commented Mar 15, 2018

cc. @Rich-Harris
I don't know if I remember right, but I seem to remember a fix in svelte where trying to set a computed prop would result in a no-op.

Maybe Rich can enlighten us... I'm on mobile now, but I'll try to find it later on

@ekhaled
Copy link
Owner

ekhaled commented Mar 15, 2018

Hmm, I was mistakenly thinking about this sveltejs/svelte#893

Seems like throwing an error is the dev mode behaviour.

Where did you get the _updatingReadonlyProperty from? Is that something internal to svelte?

@aphitiel
Copy link
Contributor Author

Thank for looking into it. Yup, it's here

@ekhaled
Copy link
Owner

ekhaled commented Mar 16, 2018

@aphitiel just had a chat with @Rich-Harris and he says what you suggest won't break anything. So feel free to open a PR.

Or just let me know if you are busy and I will sort it out.

Thanks for the investigative debugging session 😀

ekhaled added a commit that referenced this issue Mar 16, 2018
@ekhaled
Copy link
Owner

ekhaled commented Mar 16, 2018

closed in #7

@ekhaled ekhaled closed this as completed Mar 16, 2018
@ekhaled ekhaled mentioned this issue Mar 16, 2018
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

No branches or pull requests

2 participants