LottieWeb: Remove expressions eval, harden SSR check.#30872
LottieWeb: Remove expressions eval, harden SSR check.#30872CodyJasonBennett wants to merge 1 commit intomrdoob:devfrom
Conversation
| // Checks for SSR including partial DOM implementation and emulation. | ||
| // In these environments, Lottie should not be loaded or it can crash. | ||
| // https://github.com/pmndrs/three-stdlib/pull/398#issuecomment-2600778156 | ||
| const isSSR = |
There was a problem hiding this comment.
Why isn't this something apps can't check before importing lottie loader? Something like:
if ( isSSR === false ) {
const { LottieLoader } = await import( 'three/addons/loaders/LottieLoader.js' );
}There was a problem hiding this comment.
This is because three/addons brings in LottieLoader when evaluated on the server side.
I can add a CI step for this, which is as simple as node examples/jsm/Addons.js.
The changes here aren't strictly necessary for SSR (completed in #26989), only environments that partially implement DOM APIs. I wouldn't be opposed to keeping it as-is, but I've seen requests from users who use mocking in testing environments or more modern runtimes that are headless.
| // https://github.com/airbnb/lottie-web/issues/2828 | ||
| // https://github.com/airbnb/lottie-web/pull/2833 | ||
| // Bail out if we don't want expressions | ||
| function noOp(_value) { |
There was a problem hiding this comment.
It's not ideal to modify library code in this way since if we want to update lottie_canvas, we have to apply the same fixes again. I understand the argument that lottie isn't properly maintained anymore but then we should consider to remove the dependency and the loader altogether.
LottieLoader isn't complex. Users should be able to call const animation = lottie.loadAnimation( { } ); on their own and then apply lottie's canvas to a CanvasTexture.
There was a problem hiding this comment.
Okay, I volunteer to upstream any and all updates to/from Lottie Web for the next decade.
Does that really change anything, though? I don't think that's the problem.
There was a problem hiding this comment.
I see; maybe the class should be deprecated (or removed? how does this work for examples?) and the example updated to use CanvasTexture directly.
There was a problem hiding this comment.
@mrdoob originally added this loader. Let's see how he thinks about this issue.
|
Closing in favor of #30896. |
2 participants
Related issue: #29572 (closed #29572 (comment))
Description
Upstreams pmndrs/three-stdlib#415 and pmndrs/three-stdlib#398.