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

fix: use default export instead of named export #42

Closed
wants to merge 2 commits into from

Conversation

olafvanv
Copy link

@olafvanv olafvanv commented Sep 15, 2021

With this you can use Webpack 5 without warnings (or even errors in case of angular 12).
All credits to @destus90, who fixed the same thing in the esri-leaflet package (Esri/esri-leaflet#1273)

https://webpack.js.org/migrate/5/#cleanup-the-code

Using named exports from JSON modules: this is not supported by the new specification and you will get a warning. Instead of import { version } from './package.json' use import package from './package.json'; const { version } = package;

@@ -8,7 +8,7 @@
"John Gravois <[email protected]> (http://johngravois.com)"
],
"dependencies": {
"esri-leaflet": "^2.0.0",
"esri-leaflet": "^3.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change necessary? It does not seem related to the main change in this PR.

Copy link
Author

@olafvanv olafvanv Nov 17, 2021

Choose a reason for hiding this comment

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

esri-leaflet had the same issue. I suppose you have to use an updated version, otherwise you run into the same problem. See Esri/esri-leaflet#1273

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the dependency change in the PR, unless I'm missing something obvious?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

another option: jgravois/esri-leaflet-gp#46

Choose a reason for hiding this comment

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

Any update about this? Currently running into the same issue
@gavinr you mentioned that this was fixed in esri-leaflet v3.0.2 (Esri/esri-leaflet#1273 (comment)) I'm guessing that's why the dependency to esri-leaflet is updated to v3.0.2

Choose a reason for hiding this comment

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

Ran into the same issue, had to fork this library and force npm to resolve the package to my fork. Without fixing the version import and updating esri-leaflet to 3.0 the library cannot be used in projects built with webpack 5.

@gavinr
Copy link
Contributor

gavinr commented Feb 15, 2022

Thanks for the feedback everyone. It seems like there are two things that this PR is doing:

  1. Fixing the version import - this change is valid and ok with me, as we did a very similar change in esri-leaflet.
  2. Changing the esri-leaflet dependency version in package.json - I'm a bit hesitant to change this in the same PR as part 1, as I'm not sure it's related, and also there is an alternative method proposed by @jgravois.

I'd prefer that we remove the package.json changes from this PR then merge this PR. Then follow-up with a separate PR/discussion around that package.json change (preferably to change it to "peerDependencies": { "leaflet": "*", "esri-leaflet": "*" per @jgravois). If @olafvanv wants to make this change to this PR (to remove the package.json change from this PR) that would be appreciated, otherwise I'll try to tackle making these changes and the additional PR shortly.

@GVissers27
Copy link

Thanks for the feedback everyone. It seems like there are two things that this PR is doing:

  1. Fixing the version import - this change is valid and ok with me, as we did a very similar change in esri-leaflet.
  2. Changing the esri-leaflet dependency version in package.json - I'm a bit hesitant to change this in the same PR as part 1, as I'm not sure it's related, and also there is an alternative method proposed by @jgravois.

I'd prefer that we remove the package.json changes from this PR then merge this PR. Then follow-up with a separate PR/discussion around that package.json change (preferably to change it to "peerDependencies": { "leaflet": "*", "esri-leaflet": "*" per @jgravois). If @olafvanv wants to make this change to this PR (to remove the package.json change from this PR) that would be appreciated, otherwise I'll try to tackle making these changes and the additional PR shortly.

Any idea when this PR will be merged. Or are you waiting for a response?

@main-kun
Copy link

@gavinr I'm using the library right now so if it helps i can open two separate pull requests so that we don't have to wait for the author of this PR to make changes

@gavinr gavinr mentioned this pull request Feb 24, 2022
@gavinr
Copy link
Contributor

gavinr commented Feb 24, 2022

The changes in this PR have been separated out into #49 and #50. Thanks everyone for the discussion and thanks @olafvanv for the original PR.

@gavinr gavinr closed this Feb 24, 2022
@gavinr
Copy link
Contributor

gavinr commented Feb 25, 2022

This has been resolved in v3.0.0 https://github.com/Esri/esri-leaflet-cluster/releases/tag/v3.0.0

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.

5 participants