Skip to content

Fix current component not being cleared after component update #4909

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 1 commit into from
Sep 10, 2020

Conversation

rdb
Copy link
Contributor

@rdb rdb commented May 26, 2020

Fixes #4899
Fixes #4259

This PR clears the current component after the update. This has two effects:

  • The lifecycle functions now raise an error when used outside of Svelte code (eg. in a setTimeout handler). Previously, they operated on the last-updated component, which may not even be in a stable state.
  • The lifecycle functions now raise an error when used inside onMount and afterUpdate. If this is undesirable, we would need to explicitly call set_current_component in flush() again when calling the render callbacks.

This is essentially the same change as #4864, but that PR's unit test did not work correctly and left Svelte in an invalid state, causing many other unit tests to fail.

If any other unit tests or changes are needed, let me know and I'd be happy to work on them.

Closes #5077 (which is a subset of this fix that is more focused to only address #4899).

@Conduitry
Copy link
Member

@tanhauhau Does this look reasonable to you? What was your manipulation of require.cache in the test in #4864 intended to do?

@tanhauhau
Copy link
Member

Oh. Exactly what @rdb explained in #4864 (comment)

Was trying to clear the require cache of svelte/internal so the subsequent test can require a fresh state of svelte/internal

It works on mm machine though, hvnt checked why it failed in CI

@rdb
Copy link
Contributor Author

rdb commented Jun 10, 2020

Is there some way I could help to move this PR further along?

@antony
Copy link
Member

antony commented Jun 10, 2020

@rdb we just need a bit of time to understand the full ramifications of it before we merge. Apologies for any delay.

rdb added a commit to rdb/svelte-meteor-data that referenced this pull request Jun 26, 2020
Without this, unpredictable crashes may occur in seemingly random places.  This will be reverted as soon as sveltejs/svelte#4909 is merged.
@tanhauhau tanhauhau requested a review from Conduitry August 14, 2020 01:31
@Conduitry Conduitry merged commit 211fc92 into sveltejs:master Sep 10, 2020
@rdb rdb deleted the fix-current-component-not-cleared branch September 10, 2020 20:52
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants