brunob / leaflet.fullscreen

Leaflet.Control.FullScreen for Leaflet
https://brunob.github.io/leaflet.fullscreen/
MIT License
376 stars 107 forks source link

Update UMD definition #103

Closed Raruto closed 2 years ago

Raruto commented 2 years ago

The current definition does not allow an easy usage in module contexts, for example the following throws "root" is undefined error:

<script type="text/javascript">

  import('https://unpkg.com/leaflet.fullscreen').then(()=>{ /* do stuff */ });

</script>

also when using bundlers (eg. rollup) you requires some special caveats, eg:

  moduleContext: { "node_modules/leaflet.fullscreen/Control.FullScreen.js": "window" },

Reference:

Alternatively you could use the globalThis variable as well:

Have a nice day, Raruto

brunob commented 2 years ago

Thx for the PR and the explanation, any thoughts on this @BePo65 ?

BePo65 commented 2 years ago

I do not completely understand in what context @Raruto uses this plugin.

The current definition does not allow an easy usage in module contexts

Are you trying to use it in an esm context?

Besides this, I have to run some tests to find out, if the proposed changes do have side effects (that I do not expect). Give me a few days for this :-)

brunob commented 2 years ago

Besides this, I have to run some tests to find out, if the proposed changes do have side effects (that I do not expect). Give me a few days for this :-)

Again, thx a lot @BePo65 since i'm far away all these considerations.

Raruto commented 2 years ago

@BePo65 currently i am using it as static import within the leaflet-ui plugin (but to work properly with rollup it still requires you to use the moduleContext transformation, which essentially rewrites the this keyword to window). Anyway, I'm experimenting with dynamic imports, in a similar manner to the following:

<script type="text/javascript">

  import('https://unpkg.com/leaflet.fullscreen').then(()=>{ /* do stuff */ });

</script>

And also in this case, since we are always in a module context (esm), the variable this is undefined.

https://github.com/brunob/leaflet.fullscreen/blob/d1f12fa4432bb2225979ff4051e65e647056b7d1/Control.FullScreen.js#L18

Give me a few days for this :-)

No hurry, I noticed it years ago and I never thought about doing a pull request

Raruto

BePo65 commented 2 years ago

Made some tests with web browser and commonjs and as expected there are no errors. So @brunob I would recommend to merge this pr.

And I would be glad to get some feedback from @Raruto, if errors should appear after publishing this modification.

brunob commented 2 years ago

Thx for the feedback @BePo65 ! I'll merge and release this one of these days :)

brunob commented 2 years ago

Merged and released as 2.3.0 thx @Raruto & @BePo65 :)