better error: show a warning/error when server-preloaded data can't be transmitted to the client

[babichjacob]

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

• It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.: There are no directly related issues for this because it is already semi-documented behavior. That being said, there is no runtime error/warning when this happens. Tangentially related issues: Preload re-run upon hydration if server preload returned falsy #843, Exception in preload leads to flash of content #710 (another instance where re-rendering was unwanted from messing up preload), Provide warning if store data cannot be serialized by devalue  #447 (resolved issue that never got closed by the way; there is already an error for the same situation with the session store)
• Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!): There is no way to mock/assert console output in mocha/assert without introducing another dependency like sinon. By the way, there is no longer a lint package script.

Tests

• Run the tests tests with npm test or yarn test: 1 test was already failing and this doesn't change that

Like was said in #447, if there is data in preload's return that cannot be serialized by devalue (like a function in my case), the client will not receive a copy. There are no warnings for this (i.e. this is a completely silent failure), which was a source of confusion for me, despite this behavior being documented. (Actually, another PR to specify what it means when serialization fails could be needed since it is not explicitly stated that preload runs again client-side rather than displaying an error page).

This pull request introduces an error (where serialization failed and why) + warning (consequences and more information) message combo in the server side console while remaining silent in the client side. This approach was taken to remain backwards compatible and not bother the client with the server's mistakes.

Possible problems or improvements still needed with this:

• There are two console calls (error then warn) rather than one
• The warning could be restricted to dev
• Being silent on the client is different behavior than what happens when the session store's serialization fails, which displays an error page (see 2 lines later in the same file)

[antony]

This looks good. Is there a good way to add a test for this?

[babichjacob]

This looks good. Is there a good way to add a test for this?

Sure; see the latest commit.
One outstanding problem I have with it is that it's dependent on devalue's particular error message. Would it be better to replace the assert.equal call with assert.ok((await r.text('p')).startsWith('Failed to serialize preloaded data: ')) to remove that dependency?

[Conduitry]

I believe the original intention of this behavior was that, if there were no preload data sent from the server to the client (for example, if it could not be serialized), the client-side preload would be also be run to try to re-create it. I don't think this should be an outright error.

Also sort of adjacent: #843.

[babichjacob]

Does that mean the most preferred behavior was in commit 427a6bc and the ones added since then should be dropped from this PR?

(In which case, this stops being a breaking change)

[benmccann]

How about if we error out with a hard fail if we're in development mode, but allow the client to re-render over the top of it in production mode? That would make it easier to notice something's wrong in development, but not error unless necessary in production

[benmccann]

Also, we made a very similar change in 0.27.14 in #719. Perhaps we'd want it to do the same?

[Conduitry]

Having this be an error, even in dev mode, seems to indicate that we're telling the user they're doing something objectively wrong that we don't want to support, but that, for some technical/performance/whatever reason, we don't want to enforce it in production builds. I'm okay with making this a warning, but, again, making this be an error at all (even in dev mode) marks a conscious change in how we're saying this feature should work, and I don't think that change is warranted. It's perfectly possible to have a working (although non-optimal) app that re-runs preload on the client.

[benmccann]

If you wanted to run something only on the client, I'd expect you'd put it in onMount instead of preload? Or put a check in preload for window or something to see if you're running on the client or server. I wouldn't expect to be able to write code that always fails on the server and then have the failure be ignored

[pngwn]

I don't think this should be an error, this is a pretty significant breaking change and I don't think any of the arguments in favour are particularly convincing. Certainly not convincing enough to make this change without further discussion on what the behaviour should be. This is the kind of breaking change that could require significant investment from users to fix and offers very little value in exchange.

I'd suggest going with a warning for now, if anything. At the very least we need a simple way for users to log these errors to their error reporting system before issuing this change.

[Conduitry]

preload prevents the component from loading at all (and delays navigation) until the promises resolve, which onMount cannot do. IMO you're not writing code that fails on the server, you're writing code that returns a value that we can't serialize on the server.

Making this be an error feels to me like a frivolous, puritanical thing for Sapper to do.

[benmccann]

I'm okay with the warning behavior @babichjacob initially proposed if everyone else is. I made a backup of this branch and then rewound it to the initial commit. Let me know if it looks okay now

[ajbouh]

As a user I've been very surprised in a bad way by the existing behavior. It's only after I add a lot more logging that I realize that I'm preloading twice because I'm accidentally trying to serialize something that can't be serialized. By swallowing the failure we put people that are trying to build fast, efficient services at a meaningful disadvantage. :/

…

On Sun, Aug 2, 2020, 17:14 Conduitry ***@***.***> wrote: preload prevents the component from loading at all (and delays navigation) until the promises resolve, which onMouint cannot do. IMO you're not writing code that fails on the server, you're writing code that returns a value that we can't *serialize* on the server. Making this be an error feels to me like a frivolous, puritanical thing for Sapper to do. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1304 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAABZHPVRH6PFDNG2JSPVNTR6X6NTANCNFSM4OQW57OQ> .