Skip to content

Fixes #2 #35

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

Merged
merged 2 commits into from
Jul 8, 2021
Merged

Fixes #2 #35

merged 2 commits into from
Jul 8, 2021

Conversation

vatro
Copy link

@vatro vatro commented Jul 1, 2021

Fixes "Maximum call stack size" errors when using accessors / prevents infinite loop.
The comment above the line causing the infinite loop ...
https://github.com/rixo/svelte-hmr/blob/00c4d8d94f167dec3edaf9cf1a44cee7021e889b/runtime/proxy.js#L111-L112
... implies it was added "just in case if ... something", so I think it should be pretty safe to comment/remove it (?) 🤔

@vatro vatro mentioned this pull request Jul 1, 2021
3 tasks
@rixo
Copy link
Collaborator

rixo commented Jul 5, 2021

Good catch, thanks a lot.

I don't think the "who knows?" comment was meant like "in case of" but, looking at it now, I do feel it was misguided. I believe the intent was that the prop on the proxy, that is what the outside world sees, remains the same value as what was set on the "internal" component. But's that's exactly what the getter gives us so, yeah, I don't see the point and I think we can remove it safely (advantageously even).

If that's correct, we should just remove the 2 lines (including the comment above the line you've commented out) instead of commenting out. Can you update the PR? (I can do it myself, but it feels like you deserve authorship of the fix.)

@vatro
Copy link
Author

vatro commented Jul 6, 2021

Cool! I'm happy I could contribute! 😁 Keep up the great work! 👍🚀 + Thanks!

@rixo rixo merged commit efb844b into sveltejs:master Jul 8, 2021
@rixo
Copy link
Collaborator

rixo commented Jul 8, 2021

Released as [email protected] (next).

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.

2 participants