Support SSR #9317

matthova · 2024-04-06T17:08:16Z

This PR is an alternative to #9316
It doesn't enforce nullish DOM access via lint, but, it does work 😅👍

matthova · 2024-04-06T17:17:19Z

@IvanSanchez Unfortunately optional chaining does not work if the window is undefined. The current pattern I'm using is to check via typeof

const pointer = typeof window === 'undefined' ? false : !!window.PointerEvent;

jonkoops · 2024-04-06T17:21:39Z

To get back to the discussion under #9316.

My use case, as a consumer, is that I love being able to install a package and not have to think about it or do any additional configuration.

Whilst I understand this concern and we want to make using Leaflet as user-friendly as possible, I am left wondering what the implications of this are? I am not sure about adding support for SSR when Leaflet is very much not made to ever be used in a server-side context.

What will this PR accomplish exactly? Will this cause Leaflet code be executed on the server? If so, this would yield additional overhead of code execution that does essentially nothing. I cannot imagine this will actually get as far as to render the map on the server and re-hydrate it on the client. In which case I might even consider it a feature that Leaflet doesn't work in such a context.

Could you provide a minimal example of how Leaflet would be integrated in a Remix application, what the roadblocks are you are running into and how this will resolve said problem without introducing unnecessary overhead?

matthova · 2024-04-06T17:51:21Z

@jonkoops I have a lot of thoughts about Remix, but the bigger picture is that this is a problem that would need a unique solution across all of the current fad of server-rendered UI frameworks, NextJS, SvelteKit, Remix, Nuxt, etc... (I swear some of these framework names are made up 😅 )

In all of these style of frameworks, there is a server that does a first pass of the UI, and because the server doesn't have global DOM variables, SSR support puts the onus on library devs to handle nullish global dom variables. It's not ideal, but SSR support removes a big painpoint for people using libraries in these scenarios.

I take the mental load for maintainers seriously, so I understand wanting to avoid any unnecessary features. Looking at the diff in this PR, I'm cautiously convinced this is minimal lift and minimal compute for a high impact use-case. That said, the final call falls on the library devs. Thank you for the work you do 🙏🏻

Falke-Design · 2024-04-06T19:43:56Z

Looking at the diff in this PR, I'm cautiously convinced this is minimal lift and minimal compute for a high impact use-case.

I see it like this too. Over time we got a few request for ssr support and if this few changes are all that are needed, then I would add them. But I would still not publish that we support ssr. It are just a few adjustments that makes it a little bit easier without offical support.

Other SSR references:

src/core/Util.js

mourner

The changes looks good to me! Fairly minimal for enabling a rare but increasing common use case. The only thing I'd suggest is seeing if we can add some kind of a test so that we don't accidentally regress on this in future PRs (which seems easy).

jonkoops · 2024-04-09T09:11:16Z

I am still not convinced we should be landing this at all. My questions still remains, why execute Leaflet code on the server when it cannot actually do anything useful? What are the problems that will occur if we don't land this change?

From what I am seeing here we're simply introducing more variations in code branching, and taking on supporting various server-side run-times while we're at it. Which implicates that we now need to take this on as additional maintenance should these run-times have different interpretations or expediencies of how the code should be run.

For example, Deno has a window object, so the guard code written in this PR will not work. And who knows what other run-times will come up with?

I have a lot of thoughts about Remix, but the bigger picture is that this is a problem that would need a unique solution across all of the current fad of server-rendered UI frameworks, NextJS, SvelteKit, Remix, Nuxt, etc...

I understand that different SSR frameworks have different ways of dealing with client-side only code, but they all have escape hatches that allow users to run code on the client only (see Next.js, Remix and SvelteKit documentation). If a user is committing to use a framework, I think it's a valid expectancy they learn how to use it.

mourner · 2024-04-09T09:28:38Z

@jonkoops I'm not familiar with various nuances of SSR frameworks, but we had a similar situation in Mapbox GL JS — tons of requests from users to make it possible to load the bundle in a server-side environment for who knows what. Eventually we gave up and merged a similar PR and added a test which simply checked that node mapbox-gl.js doesn't throw any errors, and there were no issues or any kind of maintenance burden since.

Requiring Leaflet in some server script might not be a use case that makes a lot of sense for us (without much experience in this kind of SSR projects), but people will keep requesting it, and IMO it's a small price to pay.

why execute Leaflet code on the server when it cannot actually do anything useful?

Even if we don't consider actually rendering something on the server, they could at least use some utility functions Leaflet provides without resorting to another tool like Turf — geometric / geographical calculations, projections, etc.

jonkoops · 2024-04-09T09:41:32Z

Very well, if we have to land this I'll give this a proper pass. Let me take a look and put in a review.

jonkoops

There are still some globals referred to that might cause issues in a server-side run-time, but they will only create issues if DOM related APIs are used on the server, or if a Map is initialized.

Not sure what we want to do with those, handling them on a server is not really possible, and the guard code would be hundreds of lines of code.

jonkoops · 2024-04-09T09:48:40Z

src/core/Browser.js

@@ -40,15 +40,18 @@ const touch = touchNative || pointer;

 // @property retina: Boolean
 // `true` for browsers on a high-resolution "retina" screen or on any screen when browser's display zoom is more than 100%.
-const retina = (window.devicePixelRatio || (window.screen.deviceXDPI / window.screen.logicalXDPI)) > 1;
+const retina = typeof window === 'undefined' ? false : (window.devicePixelRatio || (window.screen.deviceXDPI / window.screen.logicalXDPI)) > 1;


This should take into account that window.devicePixelRatio and window.screen might be undefined in some run-times that do support a global window, such as Deno. Since window.devicePixelRatio is supported in all browsers we want to support for v2, I propose we drop the fallback here.

const retina = typeof window === 'undefined' || typeof window.devicePixelRatio === 'undefined' ? false : window.devicePixelRatio > 1;

jonkoops · 2024-04-09T09:51:29Z

src/core/Browser.js


 // @property mac: Boolean; `true` when the browser is running in a Mac platform
-const mac = navigator.platform.startsWith('Mac');
+const mac = typeof navigator === 'undefined' ? false : (navigator?.platform ?? '').startsWith('Mac');


This is a personal preference, but I prefer to guard the clause rather than to fall back to default values.

Suggested change

const mac = typeof navigator === 'undefined' ? false : (navigator?.platform ?? '').startsWith('Mac');

const mac = typeof navigator === 'undefined' || typeof navigator.platform === 'undefined' ? false : navigator.platform.startsWith('Mac');

jonkoops · 2024-04-09T09:53:00Z

src/core/Browser.js


 // @property mac: Boolean; `true` when the browser is running in a Linux platform
-const linux = navigator.platform.startsWith('Linux');
+const linux = typeof navigator === 'undefined' ? false : (navigator?.platform ?? '').startsWith('Linux');


Same here, I'd prefer a guard clause than a fallback.

Suggested change

const linux = typeof navigator === 'undefined' ? false : (navigator?.platform ?? '').startsWith('Linux');

const linux = typeof navigator === 'undefined' || typeof navigator.plaform === 'undefined' : navigator.platfom.startsWith('Linux');

jonkoops · 2024-04-09T09:54:36Z

src/core/Browser.js


 function userAgentContains(str) {
+	if (typeof navigator === 'undefined') {


Although supported in Deno, I am not sure navigator.userAgent might be defined in other run-times, so we might as well guard this code.

Suggested change

if (typeof navigator === 'undefined') {

if (typeof navigator === 'undefined' || typeof navigator.userAgent === 'undefined') {

jonkoops · 2024-04-09T09:57:08Z

src/core/Util.js

+const requestFn = typeof window === 'undefined' ? falseFn : window.requestAnimationFrame;
+const cancelFn = typeof window === 'undefined' ? falseFn : window.cancelAnimationFrame;


The guard here should also be more extensive, as window might be defined, but it's members not.

Suggested change

const requestFn = typeof window === 'undefined' ? falseFn : window.requestAnimationFrame;

const cancelFn = typeof window === 'undefined' ? falseFn : window.cancelAnimationFrame;

const requestFn = typeof window === 'undefined' || typeof window.requestAnimationFrame === 'undefined' ? falseFn : window.requestAnimationFrame;

const cancelFn = typeof window === 'undefined' || typeof window.cancelAnimationFrame === 'undefined' ? falseFn : window.cancelAnimationFrame;

@jonkoops I don't think we should be that thorough here — IMO we should do the minimum possible change that makes the bundle itself execute without errors, disregarding how it will be used in such environment. Putting too much guards just bloats the code for cases no one will run into.

Fair, just want to prevent that in this case things become polymorphic. In this case function and undefined.

jonkoops · 2024-04-09T09:59:17Z

src/dom/DomUtil.js

@@ -78,7 +78,7 @@ export function getPosition(el) {
 	return positions.get(el) ?? new Point(0, 0);
 }

-const documentStyle = document.documentElement.style;
+const documentStyle = typeof document === 'undefined' ? {} : document.documentElement.style;


I'd rather fall back to undefined here than give the impression there is a valid object with a fallback, and then use a guard clause wherever documentStyle is accessed.

IvanSanchez · 2024-04-09T11:01:50Z

I am still not convinced we should be landing this at all. My questions still remains, why execute Leaflet code on the server when it cannot actually do anything useful? What are the problems that will occur if we don't land this change?

@jonkoops I agree with the sentiment. All that this will do is waste some RAM in JS web servers, fail some checks, and produce nothing.

On the other hand, I think of this the same way I think of old MSIE compatibility code: we don't need it for the "normal" use case, it shouldn't be there, it's ugly but, but makes things work in some minority scenario.

mourner · 2024-04-09T14:22:38Z

There are still some globals referred to that might cause issues in a server-side run-time, but they will only create issues if DOM related APIs are used on the server, or if a Map is initialized.

In summary, I think we shouldn't anticipate any possible needs on this front by doing premature changes — instead let's accept the most minimal changes possible, as they look harmless and should cover immediate bases. Users who use Leaflet in SSR contexts can always follow-up with small fixes later, which can be reviewed separately.

jonkoops · 2024-04-12T13:53:19Z

Ok, if the issues that would occur under Deno SSR are fixed this can be merged as far as I am concerned.

Gugustinette · 2024-04-24T19:55:52Z

Thanks for considering these changes ! 🥹 This is also a common discussion in packages such as Nuxt Leaflet which I work on.

We do not exactly want to run Leaflet on the server, but just having it's instance loaded without throwing errors, so SSR environments can play with it (and most of the time, send it to the client on demand).
This will make Leaflet very easy to use in these magic and modern framework.

I can try a kind of canary version of Leaflet on my module before merging if you want.

NilsBaumgartner1994 · 2024-05-04T14:40:53Z

PLease make this work :-)

Falke-Design · 2024-07-02T20:31:33Z

I accidentally closed this PR by adding the wanted changes ... I reopened it (#9385)

I added the changes from @jonkoops and made it work with Deno.

@Gugustinette can you please test the version of #9385?

Falke-Design · 2024-07-02T20:32:56Z

@matthova if its possible for you to use the commits from #9385 and reopen this PR again, I'm happy to close my request again

matthova mentioned this pull request Apr 6, 2024

Patch Leaflet to be Server-Side-Render friendly (SSR) #9316

Closed

matthova force-pushed the main branch from 1e14196 to 33b835a Compare April 6, 2024 17:10

Falke-Design requested changes Apr 6, 2024

View reviewed changes

src/core/Util.js Outdated Show resolved Hide resolved

matthova requested a review from Falke-Design April 6, 2024 21:28

mourner reviewed Apr 9, 2024

View reviewed changes

jonkoops requested changes Apr 9, 2024

View reviewed changes

Gugustinette mentioned this pull request Apr 24, 2024

consider not globally injecting leaflet nuxt-modules/leaflet#1

Closed

Malvoz added the compatibility Cross-browser/device/environment compatibility label May 19, 2024

Falke-Design force-pushed the main branch from 9fc3138 to 2d54113 Compare July 2, 2024 18:46

Falke-Design closed this Jul 2, 2024

Falke-Design force-pushed the main branch from 2d54113 to ed3c085 Compare July 2, 2024 20:05

Falke-Design mentioned this pull request Jul 2, 2024

Support SSR (Reopen #9317) #9385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SSR #9317

Support SSR #9317

matthova commented Apr 6, 2024

matthova commented Apr 6, 2024 •

edited

Loading

jonkoops commented Apr 6, 2024

matthova commented Apr 6, 2024

Falke-Design commented Apr 6, 2024 •

edited

Loading

mourner left a comment

jonkoops commented Apr 9, 2024

mourner commented Apr 9, 2024

jonkoops commented Apr 9, 2024

jonkoops left a comment

jonkoops Apr 9, 2024

jonkoops Apr 9, 2024

jonkoops Apr 9, 2024

jonkoops Apr 9, 2024

jonkoops Apr 9, 2024

mourner Apr 9, 2024

jonkoops Apr 9, 2024

jonkoops Apr 9, 2024

IvanSanchez commented Apr 9, 2024

mourner commented Apr 9, 2024

jonkoops commented Apr 12, 2024

Gugustinette commented Apr 24, 2024 •

edited

Loading

NilsBaumgartner1994 commented May 4, 2024

Falke-Design commented Jul 2, 2024

Falke-Design commented Jul 2, 2024

	const mac = typeof navigator === 'undefined' ? false : (navigator?.platform ?? '').startsWith('Mac');
	const mac = typeof navigator === 'undefined' \|\| typeof navigator.platform === 'undefined' ? false : navigator.platform.startsWith('Mac');

	const linux = typeof navigator === 'undefined' ? false : (navigator?.platform ?? '').startsWith('Linux');
	const linux = typeof navigator === 'undefined' \|\| typeof navigator.plaform === 'undefined' : navigator.platfom.startsWith('Linux');


		function userAgentContains(str) {
		if (typeof navigator === 'undefined') {

	if (typeof navigator === 'undefined') {
	if (typeof navigator === 'undefined' \|\| typeof navigator.userAgent === 'undefined') {

		const requestFn = typeof window === 'undefined' ? falseFn : window.requestAnimationFrame;
		const cancelFn = typeof window === 'undefined' ? falseFn : window.cancelAnimationFrame;

Support SSR #9317

Support SSR #9317

Conversation

matthova commented Apr 6, 2024

matthova commented Apr 6, 2024 • edited Loading

jonkoops commented Apr 6, 2024

matthova commented Apr 6, 2024

Falke-Design commented Apr 6, 2024 • edited Loading

mourner left a comment

Choose a reason for hiding this comment

jonkoops commented Apr 9, 2024

mourner commented Apr 9, 2024

jonkoops commented Apr 9, 2024

jonkoops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanSanchez commented Apr 9, 2024

mourner commented Apr 9, 2024

jonkoops commented Apr 12, 2024

Gugustinette commented Apr 24, 2024 • edited Loading

NilsBaumgartner1994 commented May 4, 2024

Falke-Design commented Jul 2, 2024

Falke-Design commented Jul 2, 2024

matthova commented Apr 6, 2024 •

edited

Loading

Falke-Design commented Apr 6, 2024 •

edited

Loading

Gugustinette commented Apr 24, 2024 •

edited

Loading