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.

Sign up for GitHub

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

Jump to bottom

Fix: unable to remove listener with layer(s) registered with Map#once #4592

Merged
merged 6 commits into from
Aug 26, 2024
Merged

Fix: unable to remove listener with layer(s) registered with Map#once #4592

merged 6 commits into from
Aug 26, 2024

Conversation

pstaszek
Copy link
Contributor

This PR fixes issue, that Map#off does not deregister an event registered with Map#once, when layer(s) are provided.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

pstaszek
pstaszek commented Aug 24, 2024
View reviewed changes
src/ui/map.ts
for (const key in delegatedListener.delegates) {
const delegate: Delegate = delegatedListener.delegates[key];
delegatedListener.delegates[key] = (...args: Parameters<Delegate>) => {
this._removeDelegatedListener(type, layerIds, listener);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern is that in this._removeDelegatedListener the Map#off is called. On the other hand here we have reference to delegate, so it shouldn't harm. As a solution for that, we can add parameter, to skip calling off in this._removeDelegatedListener.

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.87%. Comparing base (bc70bc5) to head (b4cdf10).

Files Patch % Lines
src/ui/map.ts 75.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4592      +/-   ##
==========================================
- Coverage   87.88%   87.87%   -0.02%     
==========================================
  Files         247      247              
  Lines       33601    33613      +12     
  Branches     2336     2339       +3     
==========================================
+ Hits        29530    29536       +6     
+ Misses       3108     3105       -3     
- Partials      963      972       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

HarelM
HarelM reviewed Aug 25, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
HarelM
HarelM reviewed Aug 25, 2024
View reviewed changes
src/ui/map.ts Outdated Show resolved Hide resolved
HarelM
HarelM reviewed Aug 26, 2024
View reviewed changes
src/ui/map.ts Outdated Show resolved Hide resolved
HarelM
HarelM approved these changes Aug 26, 2024
View reviewed changes
Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

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

Thanks!!

@HarelM HarelM merged commit 07d3e61 into maplibre:main Aug 26, 2024
15 checks passed
alexandresoro pushed a commit to alexandresoro/ouca that referenced this pull request Sep 22, 2024
@alexandresoro
fix(deps): update dependency maplibre-gl to v4.7.0 (#52)
c1eb6a5 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [maplibre-gl](https://maplibre.org/) ([source](https://github.com/maplibre/maplibre-gl-js)) | dependencies | minor | [`4.5.2` -> `4.7.0`](https://renovatebot.com/diffs/npm/maplibre-gl/4.5.2/4.7.0) |

---

### Release Notes

<details>
<summary>maplibre/maplibre-gl-js (maplibre-gl)</summary>

### [`v4.7.0`](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#470)

[Compare Source](maplibre/maplibre-gl-js@v4.6.0...v4.7.0)

##### ✨ Features and improvements

-   Support multiple layers in `map.on`, `map.once` and `map.off` methods ([#&#8203;4570](maplibre/maplibre-gl-js#4570))
-   Ensure GeoJSON cluster sources emit a console warning if `maxzoom` is less than or equal to `clusterMaxZoom` since in this case you may see unexpected results. ([#&#8203;4604](maplibre/maplibre-gl-js#4604))

##### 🐞 Bug fixes

-   Heatmap Fix for 3D terrain ([#&#8203;4571](maplibre/maplibre-gl-js#4571))
-   Fix Map#off to not remove listener with layer(s) registered with Map#once ([#&#8203;4592](maplibre/maplibre-gl-js#4592))
-   Improve types a bit for `addSource` and `getSource` ([#&#8203;4616](maplibre/maplibre-gl-js#4616))
-   Fix the color near the horizon when terrain is enabled without any sky ([#&#8203;4607](maplibre/maplibre-gl-js#4607))
-   Fix bug where `fitBounds` and `cameraForBounds` would not display across the 180th meridian (antimeridian)
-   Fix white flickering on map resize ([#&#8203;4158](maplibre/maplibre-gl-js#4158))
-   Fixed a performance regression related to symbol placement ([#&#8203;4599](maplibre/maplibre-gl-js#4599))
-   Fix a bug where cloning a Transform instance didn't include the `lngRange`. This caused a bug where
    using `transformCameraUpdate` caused the `maxBounds` to stop working just for east/west bounds. ([#&#8203;4625](maplibre/maplibre-gl-js#4625))

### [`v4.6.0`](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#460)

[Compare Source](maplibre/maplibre-gl-js@v4.5.2...v4.6.0)

##### ✨ Features and improvements

-   Prefer local glyph rendering for all CJKV characters, not just those in the CJK Unified Ideographs, Hiragana, Katakana, and Hangul Syllables blocks. ([#&#8203;4560](maplibre/maplibre-gl-js#4560)))

##### 🐞 Bug fixes

-   Fix right-to-left layout of labels that contain characters in the Arabic Extended-B code block. ([#&#8203;4536](maplibre/maplibre-gl-js#4536))
-   Fix 3D map freezing when camera is adjusted against map bounds. ([#&#8203;4537](maplibre/maplibre-gl-js#4537))
-   Fix `getStyle()` to return a clone so the object cannot be internally changed ([#&#8203;4488](maplibre/maplibre-gl-js#4488))
-   Fix issues with setting sky to `undefined` ([#&#8203;4587](maplibre/maplibre-gl-js#4587)))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC40Ny4wIiwidXBkYXRlZEluVmVyIjoiMzguODYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19-->

Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/52
Reviewed-by: Alexandre Soro <code@soro.dev>
Co-authored-by: renovate <renovate@git.tristess.app>
Co-committed-by: renovate <renovate@git.tristess.app>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HarelM HarelM HarelM approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pstaszek @codecov-commenter @HarelM