Expose Leaflet global for UMD version only#9880
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes Leaflet's build system to expose the global L variable only for UMD builds while exclusively using named imports for ESM versions. The changes include removing custom global handling code in favor of Rollup's built-in functionality, freezing namespaces in ESM builds to prevent extension, and updating all documentation and examples to use named imports instead of global references.
- Removes custom global handling code and leverages Rollup's UMD output configuration
- Updates all documentation examples and references to use named imports instead of global
Lreferences - Adds comprehensive test coverage for global variable exposure in UMD builds
Reviewed Changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/LeafletWithGlobals.js | Completely removed custom global handling implementation |
| build/rollup-config.js | Updated to use Rollup's built-in UMD global handling with noConflict option |
| spec/suites/core/GlobalSpec.js | Added new test suite for global variable functionality |
| spec/suites/core/GeneralSpec.js | Removed old namespace extension tests |
| Multiple documentation files | Updated all examples to use named imports instead of global L references |
| spec/context.html | Removed leaflet-bundle import mapping |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Falke-Design
left a comment
There was a problem hiding this comment.
I’m not particularly fond of these changes, as they significantly alter the behavior of the current alpha and affect compatibility with the plugins.
I would only change applying L to globalThis. In my view, everything else should be kept (except for the docs changes L.)
| format: 'es', | ||
| banner, | ||
| sourcemap: true, | ||
| freeze: false |
There was a problem hiding this comment.
Why not keeping freeze: false?
| import * as L from './Leaflet.js'; | ||
| export * from './Leaflet.js'; | ||
|
|
||
| export default L; |
There was a problem hiding this comment.
I'm not sure, but I would keep exporting L
|
|
||
| <script type="module"> | ||
| import L, {Map, TileLayer, Marker, Icon} from 'leaflet'; | ||
| import {Map, TileLayer, Marker} from 'leaflet'; |
There was a problem hiding this comment.
I still would export L for "debugging" purpose
| globalThis.L = L; // only for debugging in the developer console | ||
| globalThis.map = map; // only for debugging in the developer console |
There was a problem hiding this comment.
I want to keep this for debugging, as I added it explicitly for that purpose
|
Let's take this one at a time, otherwise we risk having a bunch of async discussion. Firstly, let's talk about the default It also makes it significantly harder for tooling to do proper dead-code elimination trough tree-shaking, once the default import is consumed, it is no longer possible for a bundler such as WebPack or Vite to determine which parts of the module are used, so it must assume that all of them are used, meaning the entirety of Leaflet is bundled. Some other dead code elimination methods can still apply, but the initial pass cannot be used to shake out dead code. I don't think the fact that we're currently shipping alphas should factor in whether API changes can still be made, that is what an alpha is for. Once we get to a release candidate phase, we should strive to no longer make any changes that could impact API stability. |
|
In regard to tree-shaking, I also see a user already reported #9814 that confirms v2 in its current state is not tree-shakable. |
01adb73 to
d261e16
Compare
Closes Leaflet#9842 Signed-off-by: Jon Koops <jonkoops@gmail.com>
Of course, the goal should be to remove the global L, but the number of users which are migrating to v2 will naturally decrease once we remove L. We are an old plugin with a large fan base, and we should not make it to hard for them.
Tree Shaking is still possible if the bundler would accesses the build file without global L (if we are able to provide a UMD script with global L and a ESM script without L)
Sure, but I'm against removing something that users would expect us to provide, especially since I already informed them that we plan to make the release in November. We have enough time to remove these things in another major release. |
|
Closing this PR, as it has too many conflicts. It could be used as a foundation for landing #10036 |
Changes the build so the Leaflet global (
L) is only exposed for the UMD version of Leaflet. The exposing of the global, and thenoConflict()mode, is now also handled fully by Rollup, instead of our own custom implementation. A bug where another global calledleafletwas accidentally exposed (also present in v1.x), is now fixed.Other notable changes:
L.Util.foo = 'bar';), the UMD version still allows this for backwards compatibility.Closes #9842