vitalets / react-native-extended-stylesheet

Extended StyleSheets for React Native
MIT License
2.93k stars 132 forks source link

EStyleSheet.subscribe callback get called only one time #109

Closed dhl1402 closed 5 years ago

dhl1402 commented 5 years ago

When style is already built, EStyleSheet.subscribe's callback will get called only one time immediately. Is this designated behavior??

vitalets commented 5 years ago

hi @dhl1402 ! No, the callback should be called on every EStyleSheet.build(). Could you show a demo code?

dhl1402 commented 5 years ago

Sorry for late reply. Totally forgot about this. Here the code of your subscribe function:

subscribe(event, listener) {
    if (event !== BUILD_EVENT) {
      throw new Error(`Only '${BUILD_EVENT}' event is currently supported.`);
    }
    if (typeof listener !== 'function') {
      throw new Error('Listener should be a function.');
    }
    if (this.builded) {
      listener();
    } else {
      this.listeners[BUILD_EVENT] = this.listeners[BUILD_EVENT] || [];
      this.listeners[BUILD_EVENT].push(listener);
    }
  }

If styles is already built, the listener will be called but will not be saved for the next EStyleSheet.build() calls. I will add a snack later 😀

dhl1402 commented 5 years ago

hi @vitalets, here the reproduce snack. please take a look 😀 https://snack.expo.io/@dhl1402/react-native-extended-stylesheet-reproduce

vitalets commented 5 years ago

I've got you point. Yes, this is a bug from the time when there was no re-build. Thanks! Listener should be saved anyway and called if styles already built:

subscribe(event, listener) {
    if (event !== BUILD_EVENT) {
      throw new Error(`Only '${BUILD_EVENT}' event is currently supported.`);
    }
    if (typeof listener !== 'function') {
      throw new Error('Listener should be a function.');
    }
    this.listeners[BUILD_EVENT] = this.listeners[BUILD_EVENT] || [];
    this.listeners[BUILD_EVENT].push(listener);
    if (this.builded) {
      listener();
    }
  }
dhl1402 commented 5 years ago

Great! I think we need a way to remove the listeners. Maybe return an unsubscribe function or return the index of listener and expose an unsubscribe function

vitalets commented 5 years ago

I think we can introduce unsubscribe function:

unsubscribe(event, listener) { }

By the way could you describe your workflow with subscription? Originally it was done for defining components on the first tick. But it seems to me there are other use-cases.

dhl1402 commented 5 years ago

I don't use subscription for defining components but for re-calculating styles after theme change as mentioned in #47