Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not rely on global L #984

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Do not rely on global L #984

wants to merge 2 commits into from

Conversation

m1gu3l
Copy link

@m1gu3l m1gu3l commented Jan 21, 2020

Should fix #874

Comment on lines +46 to +48
inject({
L: "leaflet"
})
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This injects L to each file and assigns it import "leaflet"

Comment on lines +40 to +42
globals: {
"leaflet": "L"
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tells UMD wrapper to resolve leaflet with global L

@m1gu3l m1gu3l requested a review from danzel January 22, 2020 14:31
@danzel danzel removed their request for review January 22, 2020 19:04
@pke
Copy link

pke commented Jan 23, 2020

Wicked! ;) I have no time to test it right now, but I will eventually. Interesting to see what @IvanSanchez has to say to it.

The whole leaflet ecosystem needs a serious ES6 overhaul with a major new release.

appsemble-bot pushed a commit to appsemble/appsemble that referenced this pull request Feb 18, 2020
This can be reverted once this PR gets merged:

Leaflet/Leaflet.markercluster#984
@ivansieder
Copy link

@IvanSanchez is there any way to get your review on this? That'd be awesome!

BePo65 added a commit to BePo65/bepo65.Leaflet.markercluster that referenced this pull request Feb 27, 2021
Do not rely on global L (pull request #984)
(Leaflet/Leaflet.markercluster#984)
according to [@m1gu3l](https://github.com/m1gu3l))
fix issue #874 (Does not work with ES6 imports for leaflet)
@frafra
Copy link

frafra commented Oct 10, 2021

I am sorry for bumping this, but is there any news?

@jacobweber
Copy link

Any way to get this merged? Thanks.

@frafra
Copy link

frafra commented Mar 12, 2022

@ykzeng sorry to bother you, but could you please provide feedback on this PR?

@Schleuse
Copy link

Works great for me 👍

@JayceeZ
Copy link

JayceeZ commented Mar 14, 2022

Using this change in our fork since ages.

jacobweberbowery added a commit to Bowery-RES/Leaflet.markercluster that referenced this pull request Apr 20, 2022
@jonkoops
Copy link

@mourner could you make me a collaborator so I can review PRs on this repo?

@ykzeng
Copy link
Collaborator

ykzeng commented May 22, 2023

@mourner could you make me a collaborator so I can review PRs on this repo?

@jonkoops still interested in this? I would be happy to check if I can add you

@ykzeng ykzeng self-assigned this May 22, 2023
@ykzeng
Copy link
Collaborator

ykzeng commented May 22, 2023

Hi @IvanSanchez could you kindly help review this to make sure this is compliant with the plugin guideline? Would love to merge this or hear from you otherwise, I can certainly help if any other update will be needed as you might suggest.

@jonkoops
Copy link

I am currently too busy with work on core Leaflet, so I will not have time to review this work anytime soon. I do intend to refactor most if not all of the official Leaflet plugins once we have a solid idea of what the API surface for Leaflet 2.0 will be.

@ykzeng
Copy link
Collaborator

ykzeng commented May 23, 2023

np @jonkoops let's stay in touch if you need any help hand

@chintal
Copy link

chintal commented May 9, 2024

@m1gu3l Thank you for your PR. After over a day of pulling my hair out trying to figure out why clustering was so broken between remix, react-leaflet-cluster, and leaflet.markercluster, your solution worked perfectly on the first try.

I was able to use it by :

  • manually apply your changes to a fresh fork of the latest main, (build/rollupconfig.js)
  • force installing @rollup/plugin-inject
  • building the js
  • publishing the whole package including the built js back to github and installing from there instead

Here's hoping the issue can be resolved in the package itself, at which point I can switch back and nuke the fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not work with ES6 imports for leaflet