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

Pin earcut version #12412

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Pin earcut version #12412

wants to merge 1 commit into from

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Jan 3, 2025

Description

Version 3.0.0 of earcut dropped support for CommonJS however CesiumJS still supports using it as a CommonJS module. This PR pins the version at 2.2.4 for now for compatibility.

Opened #12411 to track updating this in the future

This was the reported cause of #12294

Issue number and link

Fixes #12294

Testing plan

I'm not entirely sure how to test this with a polygon geometry that has holes @ggetz is there an easy way to make that?

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@jjspace jjspace requested a review from ggetz January 3, 2025 20:40
Copy link

github-actions bot commented Jan 3, 2025

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Jan 6, 2025

Thanks for looking into this @jjspace!

I've asked @ArgentumHHH for a Sandcastle example so we can verify this fix.

@ggetz ggetz mentioned this pull request Jan 6, 2025
@ggetz
Copy link
Contributor

ggetz commented Jan 27, 2025

I haven't been able to track down code to reproduce #12294.

@jjspace Are you sure earcut dropping CommonJS is the source of this issue? We use earcut only in our (ESM) source code where it then gets built into a worker.

@ggetz
Copy link
Contributor

ggetz commented Feb 4, 2025

@jjspace any thoughts on my comment above?

@jjspace
Copy link
Contributor Author

jjspace commented Feb 7, 2025

Are you sure earcut dropping CommonJS is the source of this issue? We use earcut only in our (ESM) source code where it then gets built into a worker.

@ggetz I am not positive, I just opened the PR quick because it seemed to be the highlighted cause and I saw it swapped to ESM only. I was hoping we could get a good sample to verify.

I was under the assumption we wanted to avoid ESM only modules because we still support CommonJS. We've had issues with that in the past. I did not look closely at where this is used so if this packages is not one we have to care about I think this PR can just be closed.

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.

PolygonGeometry with holes have broken surfaces
2 participants