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

Svelte: Fix reactivity loops & crashes #487

Merged

Conversation

pingu-codes
Copy link
Contributor

This PR implements the changes discussed in #484 which fixes bugs that occur from Svelte 5's new reactivity system.

Copy link
Contributor

@rokotyan rokotyan left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Let a few minor comments

@@ -38,6 +38,8 @@
let ref: HTMLDivElement

$: chart?.setData(data, true)
// eslint-disable-next-line no-self-assign
$: config.components = config.components
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if there are there any drawbacks of doing this, e.g. extra re-renders when nothing changes or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it actually fixes what it was originally supposed to. My original issue was that the chart wasn't rerendering when child component data changes but this doesn't actually fix that. I'm not sure if it's intended that when a child components data is set the whole graph rerenders.

If it is it'll need a different fix and I can look into it lmk :)

Copy link

github-actions bot commented Nov 26, 2024

CLA Assistant Lite bot ✅ All required contributors have signed the F5 CLA for this PR. Thank you!

@pingu-codes
Copy link
Contributor Author

I have hereby read the F5 CLA and agree to its terms

@lee00678
Copy link
Collaborator

lee00678 commented Dec 3, 2024

Just released 1.5.0-svelte.0. I tested it with a simple Svelte 5 project, seems to be working for me.

@lee00678
Copy link
Collaborator

lee00678 commented Dec 3, 2024

@pingu-codes This PR looks good. Do you mind cleaning up the commit history (part of my fault as well)? You can either squash it or overwrite the history and do a force push to the branch.

@pingu-codes pingu-codes force-pushed the 484-svelte-5-infinite-effect--reactive-l branch from 93606df to 5f6fc99 Compare December 5, 2024 11:51
@pingu-codes
Copy link
Contributor Author

Yep, just squashed the commits!

@pingu-codes
Copy link
Contributor Author

Do you think I should remove the changes to the package-lock.json on website too as it seems to include a lot of changes not really related to the fix?

@50rayn
Copy link
Contributor

50rayn commented Dec 5, 2024

I would rebase onto the latest main branch. After that, discard changes from package-lock.json and rerun an npm ci. And recheck if everything is ok and works as expected. Why npm ci, because npm i adds new dependencies and updates dependencies on a project.

@lee00678
Copy link
Collaborator

lee00678 commented Dec 5, 2024

Do you think I should remove the changes to the package-lock.json on website too as it seems to include a lot of changes not really related to the fix?

Yea, I would rebase from main. We did a major website upgrade so there will be a lot of updates in the website package-lock.json

@lee00678 lee00678 force-pushed the 484-svelte-5-infinite-effect--reactive-l branch from dc188c7 to 5f9db98 Compare December 5, 2024 23:25
@pingu-codes
Copy link
Contributor Author

Did you handle the changes with your push @lee00678 sorry I've been really busy the last few days. I'm pretty bad at github tbh (mainly use graphite at my work) so I don't want to break anything if you've made the changes you need 😅

@lee00678
Copy link
Collaborator

lee00678 commented Dec 6, 2024

Did you handle the changes with your push @lee00678 sorry I've been really busy the last few days. I'm pretty bad at github tbh (mainly use graphite at my work) so I don't want to break anything if you've made the changes you need 😅

I got it! No worries. We are good on this PR. Thanks for the contribution. I went ahead and fixed it because we want to release 1.5 on Monday.

@sumitkumar25 sumitkumar25 self-requested a review December 9, 2024 02:36
@lee00678 lee00678 merged commit a2c7faa into f5:main Dec 9, 2024
4 checks passed
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.

5 participants