Fix redundant suspense on hydration

[kinday]

Summary

loadSync does not update cache thus suspense kicks in during hydration, which causes flash of loader. This fix ensures cache is updated when loadSync is called if suspense is used to keep hydration as smooth as possible.

Test plan

⚠️ Not sure if possible to cover it with current tests as they do not seem to have anything related to output of Babel transform

Meanwhile it will be tested in one of my company’s codebases to see if the change causes any regressions, but as I understand it will not cover all possible scenarios.

[kinday]

If there are any reasons why cache was not updated in loadSync, could you share them please? Another idea of mine was to try reading this.state.result in loadAsync, which yields to same results, but presented solution seemed more appropriate to me.

[theKashey]

I am pretty sure that the "proper" fix would be to move if(options.suspense) from line 189 into if(loading) block at line 208.
Like - if you are loading something, then throw in case Suspense or display fallback otherwise.

That would prevent this issue from happening from a state machine point of view.

[kinday]

I don’t see how your proposal will fix the issue. The problem is that after loadableReady all chunks are on client, but right now if suspense is enabled, the tree is suspended for every chunk to be resolved asynchronously even if it is possible to require it synchronously.

[theKashey]

2 lines below your changes (:136)

          if (options.suspense) { // your change
            this.setCache(result)
          }
          this.state.result = result
          this.state.loading = false; <--- explicitly set loading to false

Rule is simple - if you are not this.state.loading - you are NOT loading.

[kinday]

Aha! I see now. Thank you. I’ll try it out and update PR if everything works as intended.

[kinday]

Great suggestion. It works this way. No flickering and everything seems to load correctly.

[kinday]

I’ve just thought of another issue I’ve noticed: errors were not thrown in most cases with suspense enabled (missing default export, module evaluation problems) and now they appear as they should since throw error comes first.

[theKashey]

Oh, that's good news from one point of view, and a breaking change from another (even if all tests 😅 are green).

So this time we created another problem for ourselves - state.error is never unset. While state.loading is never set, except in the constructor.

🤓 Sorry mate, look like your initial approach would cause fewer fixes, not requiring to rewrite entire loadable, however keeping it "broken" for non-suspense users in the edge cases.

Probably it's better to merge the first version, to mitigate the problem, and only then refactor everything. @gregberge - is it sounds reasonable?

[kinday]

I don’t see the problem and I don’t see how this is the breaking change. Can you elaborate?

As I see it... If suspense is disabled, everything works exactly the same as before. If suspense is enabled, it works the same as non-suspense but with suspending the tree instead of using given fallback.

[theKashey]

loading flag is set only once - in the constructor, so you will not be able to "retry" failed loading process.
You've opened this PR as long as suspense is broken for your case, after it would be broken for another, far less important, but still valid.

[kinday]

I pushed back the first version as you requested.

[kamikazePT]

The cache isnt persisted, so... When you jump to rehydration on the client side I believe you Will find yourself with an empty cache, hence throwing the promise again and suspending, no?

[gregberge]

This should be part of the refactoring to make Suspense work correctly.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.