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

Add third parameter to once event handler #8875

Merged
merged 10 commits into from
Jan 7, 2020
Merged

Add third parameter to once event handler #8875

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6d7277a
adds third parameter to once event handler
malwoodsantoro Oct 15, 2019
64bd5cb
refactor
malwoodsantoro Nov 2, 2019
c73f64f
fix lint error
malwoodsantoro Nov 5, 2019
b0d3555
fix flow error
malwoodsantoro Nov 5, 2019
6becd9a
change super.on to super.once
malwoodsantoro Nov 12, 2019
e5492f7
remove one time delegated listeners on off
malwoodsantoro Dec 10, 2019
c373a75
remove comments
malwoodsantoro Dec 10, 2019
4d5172f
add docs for once event
malwoodsantoro Dec 17, 2019
3e0cc73
utilized the _oneTimeListeners array in Evented
malwoodsantoro Dec 17, 2019
ffe7d5d
Docs fixes per review
Dec 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 68 additions & 47 deletions src/ui/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,54 @@ class Map extends Camera {
return this._mapId;
}

_createDelegatedListener(type: MapEvent, layerId: any, listener: any) {
if (type === 'mouseenter' || type === 'mouseover') {
let mousein = false;
const mousemove = (e) => {
const features = this.getLayer(layerId) ? this.queryRenderedFeatures(e.point, {layers: [layerId]}) : [];
if (!features.length) {
mousein = false;
} else if (!mousein) {
mousein = true;
listener.call(this, new MapMouseEvent(type, this, e.originalEvent, {features}));
}
};
const mouseout = () => {
mousein = false;
};
return {layer: layerId, listener, delegates: {mousemove, mouseout}};
} else if (type === 'mouseleave' || type === 'mouseout') {
let mousein = false;
const mousemove = (e) => {
const features = this.getLayer(layerId) ? this.queryRenderedFeatures(e.point, {layers: [layerId]}) : [];
if (features.length) {
mousein = true;
} else if (mousein) {
mousein = false;
listener.call(this, new MapMouseEvent(type, this, e.originalEvent));
}
};
const mouseout = (e) => {
if (mousein) {
mousein = false;
listener.call(this, new MapMouseEvent(type, this, e.originalEvent));
}
};
return {layer: layerId, listener, delegates: {mousemove, mouseout}};
} else {
const delegate = (e) => {
const features = this.getLayer(layerId) ? this.queryRenderedFeatures(e.point, {layers: [layerId]}) : [];
if (features.length) {
// Here we need to mutate the original event, so that preventDefault works as expected.
e.features = features;
listener.call(this, e);
delete e.features;
}
};
return {layer: layerId, listener, delegates: {[type]: delegate}};
}
}

/**
* Adds a {@link IControl} to the map, calling `control.onAdd(this)`.
*
Expand Down Expand Up @@ -776,53 +824,7 @@ class Map extends Camera {
return super.on(type, layerId);
}

const delegatedListener = (() => {
if (type === 'mouseenter' || type === 'mouseover') {
let mousein = false;
const mousemove = (e) => {
const features = this.getLayer(layerId) ? this.queryRenderedFeatures(e.point, {layers: [layerId]}) : [];
if (!features.length) {
mousein = false;
} else if (!mousein) {
mousein = true;
listener.call(this, new MapMouseEvent(type, this, e.originalEvent, {features}));
}
};
const mouseout = () => {
mousein = false;
};
return {layer: layerId, listener, delegates: {mousemove, mouseout}};
} else if (type === 'mouseleave' || type === 'mouseout') {
let mousein = false;
const mousemove = (e) => {
const features = this.getLayer(layerId) ? this.queryRenderedFeatures(e.point, {layers: [layerId]}) : [];
if (features.length) {
mousein = true;
} else if (mousein) {
mousein = false;
listener.call(this, new MapMouseEvent(type, this, e.originalEvent));
}
};
const mouseout = (e) => {
if (mousein) {
mousein = false;
listener.call(this, new MapMouseEvent(type, this, e.originalEvent));
}
};
return {layer: layerId, listener, delegates: {mousemove, mouseout}};
} else {
const delegate = (e) => {
const features = this.getLayer(layerId) ? this.queryRenderedFeatures(e.point, {layers: [layerId]}) : [];
if (features.length) {
// Here we need to mutate the original event, so that preventDefault works as expected.
e.features = features;
listener.call(this, e);
delete e.features;
}
};
return {layer: layerId, listener, delegates: {[type]: delegate}};
}
})();
const delegatedListener = this._createDelegatedListener(type, layerId, listener);

this._delegatedListeners = this._delegatedListeners || {};
this._delegatedListeners[type] = this._delegatedListeners[type] || [];
Expand Down Expand Up @@ -877,6 +879,25 @@ class Map extends Camera {
return this;
}

once(type: MapEvent, layerId: any, listener: any) {

if (listener === undefined) {
return super.once(type, layerId);
}

const delegatedListener = this._createDelegatedListener(type, layerId, listener);

this._delegatedListeners = this._delegatedListeners || {};
this._delegatedListeners[type] = this._delegatedListeners[type] || [];
this._delegatedListeners[type].push(delegatedListener);
Copy link
Member

Choose a reason for hiding this comment

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

This function uses Evented#once to add listeners, which means that the event listener will be added to this._oneTimeListeners. Once the event is fired, it will be removed from that list. However, it will not be removed from this._delegatedListeners`, which means that handlers are going to accumulate in this array causing memory growth.


for (const event in delegatedListener.delegates) {
this.once((event: any), delegatedListener.delegates[event]);
}

return this;
}

/**
* Returns an array of [GeoJSON](http://geojson.org/)
* [Feature objects](https://tools.ietf.org/html/rfc7946#section-3.2)
Expand Down
2 changes: 1 addition & 1 deletion src/util/evented.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export class Evented {
* @param {Function} listener The function to be called when the event is fired the first time.
* @returns {Object} `this`
*/
once(type: string, listener: Listener) {
once(type: *, listener: Listener) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should remain string. Any reason this had to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kkaefer flow fails without * and on currently uses * as well (assuming for similar reasons).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, flow seems to have trouble matching a string to a string literal :/

this._oneTimeListeners = this._oneTimeListeners || {};
_addEventListener(type, listener, this._oneTimeListeners);

Expand Down