Patch Leaflet to be Server-Side-Render friendly (SSR) #9316
Conversation
docs/docs/js/docs.js
Outdated
| @@ -2,85 +2,92 @@ | |||
| hljs.configure({tabReplace: ' '}); | |||
| hljs.initHighlighting(); | |||
|
|
|||
| const tocCopy = document.createElement('div'); | |||
| tocCopy.id = 'toc-copy'; | |||
| function setup() { | |||
There was a problem hiding this comment.
Anything in docs/ will only ever run in a browser. You made the blanket assumption that all .js files are part of the Leaflet code, but some are part of its website. These changes are therefore too broad.
| mac, | ||
| linux | ||
| }; | ||
| export default setupBrowser(); |
There was a problem hiding this comment.
This breaks tree-shaking (which, from my PoV, is more important than SSR friendlyness)
There was a problem hiding this comment.
Good callout. I'll see if there's a way to have both
| const pointer = !!window.PointerEvent; | ||
| // @property pointer: Boolean | ||
| // `true` for all browsers supporting [pointer events](https://msdn.microsoft.com/en-us/library/dn433244%28v=vs.85%29.aspx). | ||
| const pointer = !!window.PointerEvent; |
There was a problem hiding this comment.
You may use something like !!(window?.PointerEvent). Ditto for navigator and the like.
|
@IvanSanchez thank you for the feedback! I love the idea of enforcing ssr-compatibility via the lint plugin I added, however, I wonder if there's a slightly different approach we could take for this library to remove unnecessary function-wrapping. Let me do some digging |
I am curious about this. Why would Leaflet need to be 'SSR friendly' in order for this to work? Leaflet was always intended to be a complete client-side solution, so why even use it in an SSR context? Can Remix not opt-out of SSR for the few places one might need to include Leaflet? |
That's a great question. I think the maintainers of the library need to own this decision. 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. The tradeoff may not be worth it, for maintainers. |
|
Closing and will submit an alternate approach with optional chaining |
|
Alternate PR: #9317 |
This Pull request adds the eslint plugin
ssr-friendlyand implements changes to meet the new linting requirements.I've confirmed I'm able to use the forked library with a SSR Remix app.