jgkim / react-native-status-bar-size

Watch and respond to changes in the iOS status bar height
https://www.npmjs.com/package/react-native-status-bar-size
125 stars 33 forks source link

Fix subscription management leaks #18

Closed jamesreggio closed 7 years ago

jamesreggio commented 7 years ago

Prior to this PR, event subscriptions were being stored in a plain Object, using the handler function as the key. Functions cannot be used as object keys, but when you attempt to do so, instead of receiving an error, toString() is called on the function and the resulting string is used as the key. Most functions return the same value for toString(), so only one event handler was ever being maintained at a time. The aliasing of all handlers to the same slot in the object was leading to leaks and premature subscription cancellation in removeEventListener.

This PR uses a Map for bookkeeping, which permits the use of functions as keys. It also fixes the problem raised in #17 where handlers for different event types could potentially be aliased, leading to similar premature unsubscription issues.

jamesreggio commented 7 years ago

Ping @jgkim.

jgkim commented 7 years ago

@jamesreggio Sorry for the delayed response. Actually, I cannot understand exactly what problem #17 raises. Could you elaborate on it more?

jamesreggio commented 7 years ago

Hi @jgkim, it looks like #17 fixes the issue I've fixed here.

Basically, here's what the old code was doing:


const subscriptions = {};

function addHandler(event, handler) {
  const subscription = NativeModule.addListener(event, handler);

  // typeof handler === 'function'
  subscriptions[handler] = subscription;

  // You cannot use a function as the key for an object.
  // When you do, JS will call `toString()` on the function and use that as the key instead,
  // so the value of subscriptions will look something like this: { 'function handler() { ... }': subscription }
  // This is bug-prone, since each JS runtime defines a different toString() implementation for functions.
}