[fix] bind:this works during onMount callback in manually-created component

[rmunn]

If a component was instantiated with new Component(), its onMount callback was being called before the bind:this bindings were set up, so that the binding variable was undefined. Now the bind:this bindings are properly bound before the onMount callbacks are called.

Fixes #6760.
Fixes #6919.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with [feat], [fix], [chore], or [docs].
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with npm test and lint the project with npm run lint

Note that while this change makes the regression test I added (reproducing #6760 and #6919) pass, it makes the following tests fail:

• await-catch-no-expression
• await-then-catch-multiple
• await-then-no-expression

Somehow these tests are dependent on the current behavior of flush() that is causing #6760 and #6919. I have not yet found a way to fix those two issues and still have those three await-related tests pass, so I would need a little help figuring that out.

Update: All tests are passing now. This is ready to be reviewed and merged.

[rmunn]

BTW, the current behavior of flush that I mentioned in the PR description is that if flush() is called while it is already running, it immediately returns without doing anything. The behavior prior to #4356 being merged was that flush() was reentrant. This PR essentially reverts #4356 and makes flush() reentrant again, but using a queue for dirty_components instead of an array so that dirty_components.shift() can be amortized O(1) instead of O(N). (The inefficiency of calling shift() N times in a while loop was the motivation for #4356, so I tried to maintain that efficiency.)

[rmunn]

I can fix the three await tests with the following patch, but then the onMount-get-current-component test fails:

diff --git a/src/runtime/internal/scheduler.ts b/src/runtime/internal/scheduler.ts
index cc93a6af5..fd69256a7 100644
--- a/src/runtime/internal/scheduler.ts
+++ b/src/runtime/internal/scheduler.ts
@@ -1,5 +1,6 @@
 import { run_all, Queue } from './utils';
 import { set_current_component } from './lifecycle';
+import { get_current_component } from '.';
 
 export const dirty_components = new Queue<any>();
 export const intros = { enabled: false };
@@ -37,12 +38,16 @@ export function flush() {
        do {
                // first, call beforeUpdate functions
                // and update components
+               let current_component = null;
+               try {
+                       current_component = get_current_component()
+               } catch {}
                while (dirty_components.length) {
                        const component = dirty_components.shift();
                        set_current_component(component);
                        update(component.$$);
                }
-               set_current_component(null);
+               set_current_component(current_component);
 
                dirty_components.length = 0;
 

The behavior of resetting the current component to null seems to be a deliberate feature of #4909, which means that there seems to be a conflict between three features:

• [feat] allow shorthand then and catch with no expression #6564 works if flush is not reentrant, or if flush is reentrant and does not clear the current component
• Fix current component not being cleared after component update #4909 works if flush is not reentrant, or if flush is reentrant and does clear the current component
• This PR works if flush is reentrant

So if we make flush reentrant, either #6564 or #4909 will fail. If we do not make flush reentrant, this PR will fail and #6760 and #6919 will not be fixed.

Paging @tanhauhau who can help me understand why #6564 fails when flush clears the current component. Because it sounds like #4909 absolutely relies on that behavior, so I have to find a way to make #6564 work without losing the set_current_component(null) behavior.

[rmunn]

Found a way to make all the unit tests pass, by moving the "save and restore current component" outside the do ... while loop so that it only runs once. This seems to fulfill all the requirements of the previous work on the flush() function:

• Calling get_current_component() during onMount() still throws an exception (Fix current component not being cleared after component update #4909)
• flush() is now reentrant again, undoing the if (flushing) return change from Avoid using shift in flush #4356
  • See below for why this is OK
• Tests from [feat] allow shorthand then and catch with no expression #6564 still pass

Reasons why undoing the if (flushing) return change from #4356 is okay:

• The motivation for Avoid using shift in flush #4356 was efficiency: shift() is O(N) on an array, and flush() calls it O(N) times
• The if (flushing) return change in PR 4356 was required because the change from a while loop to a for loop in that PR caused an infinite loop when flush() was called reentrantly (see PR 4356 discussion for details)
• Making flush() non-reentrant was not the main motivation for PR 4356
• With this PR, dirty_components is now a queue instead of an array, allowing shift() to work in amortized O(1) time
• This PR brings back the while loop that existed before PR 4356, so that it's safe for flush() to be reentrant again
• A reentrant flush() is necessary for bind:this to function properly during onMount()

This should be ready for someone to review and merge now.

[rmunn]

While I wait for a Svelte team member to have time to review this, a couple words about the Queue implementation. I chose to go with a deliberately minimal implementation, with only the bare minimum features needed to fulfill the needs of the dirty_components queue. So I only implemented push, shift, and length. I probably didn't need to implement the ability to set the length of the queue to 0 (clearing it), but I did need to implement a getter on length and it seemed silly not to make it a getter/setter. (Update: I did need to implement setting the length to 0, I'd just forgotten that fact when I wrote the previous sentence). But there's no iterators and no index lookup, because they're not currently needed in the flush() implementation. I figure if they're needed in the future, they can be added later. You Ain't Gonna Need It and all that.

[rmunn]

One unit test timed out after 15 minutes, apparently because the test runner got stuck. I don't have the access rights to re-run the tests. Could someone push the "Re-run all checks" button for me? Or I could push a meaningless rebase to force the tests to re-run, but that seems a little silly.

[benmccann]

We basically just ignore it if it's only a single job failing since the tests are known to be flaky

[rmunn]

We basically just ignore it if it's only a single job failing since the tests are known to be flaky

Good to know. Turned out I'd accidentally missed removing one blank line when I removed the code block above it, so I did end up pushing a commit making no code changes, which ended up re-running all the unit tests. And then one of the Mac test runners had a browser that died, which cancelled seven different jobs. So it ended up being a bit of a moot point, because I did have to do a commit --amend and a force-push in order to get the tests looking greenish again. (This time two Windows tests timed out, on different Node versions than the one that timed out earlier).

Now that I've simplified the code by removing the Queue implementation (thanks to the good suggestion @dummdidumm made), I think most of this PR will be easy to approve. There are two points remaining that I'd like someone to weigh in on:

• How I should handle saving and restoring the current component inside flush(). Three possible ways of doing it:
  • (Current state of the code): access current_component directly inside flush(), without using get_current_component()
  • Call get_current_component() and catch the error if it's undefined/null
  • Create a new maybe_get_current_component() function that doesn't throw, and use that
• Is having flushidx++ on its own line a coding style that the Svelte team likes? Or would you prefer that I move it inside the array lookup, so that it becomes const component = dirty_components[flushidx++] in one line instead of two?

Pros and cons of the ways to save and restore the current component:

• Direct access. Pro: Simple, doesn't create new code. Con: if get_current_component() grows more logic in the future, flush() won't benefit from it.
• Throw and catch. Pro: doesn't create new code in lifecycle.ts. Con: expensive in performance: will be catching and ignoring errors quite frequently.
• New maybe_get_current_component() function. Pro: if get_current_component grows more logic in the future, it will be clear that maybe_get_current_component should do the same. Con: adds a new function in lifecycle.ts, and it will probably be unclear in the future why these two functions differ.

[dummdidumm]

Thanks for your work on this, and especially for laying out your thought process this clearly.

My opinion on your questions:

• I agree that it's more readable to do it on its own line. On the other hand, it saves a few bites if you do it inline. I'm a little indifferent but lean more to the "more readable" side
• current component: Due to the bundle size concerns just mentioned I'd go with accessing the variable directly, maybe adding a little explanation why get_current_component isn't used (might throw frequently in here which is a performance hit)

In general, while we're at it, this specific method could use some more comments in my opinion. I feel like you looked at this function from all angles by now and can probably confidently add some comments explaining why things are done the way, for example that flush can be reentrant (and why), why the index needs to be tracked outside of the functions, etc.

[rmunn]

@dummdidumm wrote:

In general, while we're at it, this specific method could use some more comments in my opinion. I feel like you looked at this function from all angles by now and can probably confidently add some comments explaining why things are done the way, for example that flush can be reentrant (and why), why the index needs to be tracked outside of the functions, etc.

Done in commit fd7586c. Let me know if that's a bit too much and I'll trim the comments down a little.

[rmunn]

Note #6016 - I need to write another unit test to see if this PR would fix #6016 or whether that bug needs a different fix. Because #6858, intended to fix #6016, might conflict with this PR. More research needed.

[rmunn]

Pulled #6858's unit tests into my branch and ran them, and my changes do not fix the bugs that #6858 fixes, where beforeUpdate gets called twice. I then pulled #6858's changes into my branch and abandoned my branch's changes to flush(), keeping my branch's unit tests for bind:this being null, and #6858 does not fix the bug that this PR fixes.

So both this PR and #6858 are needed as they fix different issues. Whichever one is merged first will cause a merge conflict in the other one, but they should be simple enough to resolve.

[dummdidumm]

I like the comment! By now (and through the comment) I think I understood enough of this part to give my approval on this PR 👍 If this one's merged, you could then look at the other PR you mentioned since you are already so familiar with that part, if you like.

[rmunn]

Another issue that I should investigate is #6921. I expect it wouldn't be fixed by this PR, but it's quite possible that #6858 would fix it.