fix: document validation for SSR

[IcaroG]

Fixes #2739 #3047 #3077

[IcaroG]

Hello @bodymovin. Could you please review this PR? Thanks.

[onedrop]

please review and merge this @bodymovin

[IcaroG]

Hey @bodymovin, would it be possible to merge this PR?

[korywka]

I think we just need to fix addNavigatorValidation to check document and not navigator, cause nodejs now have navigator too. Thats why this fix stopped working.

Also to stop copy paste 'inject-version' name, cause it is not injecting version :)

[korywka]

@mbasaglia Sorry for tagging you, just maybe you can help to merge this PR. People are really suffering from this problem. (I can provide a fix for the comment above).

[mbasaglia]

@mbasaglia Sorry for tagging you, just maybe you can help to merge this PR. People are really suffering from this problem. (I can provide a fix for the comment above).

I'm not familiar with rollup so I don't know what the fix does

[fpapado]

@mbasaglia Sorry for tagging you, just maybe you can help to merge this PR. People are really suffering from this problem. (I can provide a fix for the comment above).

I'm not familiar with rollup so I don't know what the fix does

In case it helps: addDocumentValidation is a Rollup plugin, that runs in the renderChunk phase (essentially when rollup writes one "chunk" of JS, usually a file). In this case, it prefixes all code with the equivalent of:

(typeof document !== 'undefined') && /* some IIFE here, according to the build */

This line is modelled after the existing setup (addNavigatorValidation also a rollup plugin), which adds the (typeof navigator !== 'undefined') check. Using that existing setup as an example, here is how the currently-published lottie output looks (from the npm code explorer for lottie-web > build/player/lottie.js; no deep links unfortunately 😅 ), after the existing addNavigatorValidation plugin transform:

Essentially, the change in this PR injects an extra check as part of the build process, to avoid things erroring out in node versions that expose a global navigator (thus leading to an incorrect environment detection). You can verify this transform by running the build, and inspecting the output for that check.

The committer of this PR could have opted to change the navigator check plugin to a document check plugin, but the additional plugin ensures that the navigator check still gets made. For example, there might be code inside the block that assumes that navigator exists, so this seems like a reasonable choice.

I would be happy to follow-up in this or another PR if you want. At work, we are currently maintaining a patch to make lottie-web compatible with recent Node versions, and we would very much like to see something like this upstreamed 😌

Apologies for the drive-by comment, you might know some of these already and I thought I might be of some help. Please let me know if there is any information that I can provide, and I will get back to you promptly!

[IcaroG]

I've changed the PR to fix the plugin names, but kept both the validations for now.
LMK if you need any change to merge this.

[schinery]

@AliT3 @geomaster sorry to tag you on this PR but you seem to be active in this repo, so was wondering if you'd be able to take a look at this and hopefully get it merged and released?

Would make a bunch of people trying to upgrade to the latest Node very happy 😄

[schinery]

@AliT3 @geomaster sorry to tag you on this PR but you seem to be active in this repo, so was wondering if you'd be able to take a look at this and hopefully get it merged and released?

Would make a bunch of people trying to upgrade to the latest Node very happy 😄

Or #3129

[AliT3]

Looks good to me! Thanks for this fix @IcaroG

I'll merge now.