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

include route in args for willFocus/didFocus #91

Merged

Conversation

reergymerej
Copy link
Contributor

This address #90, providing access to the route along with route.name so the user can query the route for whatever is needed.

@davidLeonardi
Copy link
Contributor

If this isn't a breaking change this is a welcome contribution, which should be able to be merged pretty smoothly.

I personally depend on the name in some of my code, but i've long wished for having an actual UID for stuff.

@charpeni , opinions?

@reergymerej
Copy link
Contributor Author

In lieu of #58, I tried out this change in my project that's currently depending on the name arg. It's all working fine and now opens up another option. It should be fully backward compatible.

@davidLeonardi
Copy link
Contributor

While i like that it's backwards compatible, i think that emitting anything else but a single object is potentially misleading. I wasn't even aware that .emit supports a third argument that way.

The implementation for emitter is as follows:

/**
   * Emits an event of the given type with the given data. All handlers of that
   * particular type will be notified.
   *
   * @param {string} eventType - Name of the event to emit
   * @param {...*} Arbitrary arguments to be passed to each registered listener
   *
   * @example
   *   emitter.addListener('someEvent', function(message) {
   *     console.log(message);
   *   });
   *
   *   emitter.emit('someEvent', 'abc'); // logs 'abc'
   */
  emit(eventType: String) {
    var subscriptions = this._subscriber.getSubscriptionsForType(eventType);
    if (subscriptions) {
      var keys = Object.keys(subscriptions);
      for (var ii = 0; ii < keys.length; ii++) {
        var key = keys[ii];
        var subscription = subscriptions[key];

        // The subscription may have been removed during this event loop.
        if (subscription) {
          this._currentSubscription = subscription;
          subscription.listener.apply(
            subscription.context,
            Array.prototype.slice.call(arguments, 1)
          );
        }
      }
      this._currentSubscription = null;
    }
  }

@reergymerej
Copy link
Contributor Author

@SEthX For consistency, each emitted event should pass the route. That would allow developers to use it however they want, but it would obviously be a breaking change.

@davidLeonardi
Copy link
Contributor

I second that @reergymerej .
So, should we create the first breaking release?
Better early than too late. I believe this is an important issue, as it allows LOTS of room for change in the future.
@charpeni ? @ericraio ?

@davidLeonardi davidLeonardi added this to the 0.9.0 milestone Mar 3, 2016
@reergymerej
Copy link
Contributor Author

I'll send another PR to provide route alone instead of as a 2nd arg.

@charpeni
Copy link
Contributor

charpeni commented Mar 3, 2016

I'm agree with that. 👍

Thank you @reergymerej

@davidLeonardi
Copy link
Contributor

So, merge?

@reergymerej
Copy link
Contributor Author

semver FTW!

@charpeni
Copy link
Contributor

charpeni commented Mar 4, 2016

Should we put that into a branch named v0.9.0 waiting for the other changes ?

@davidLeonardi
Copy link
Contributor

We could, but we can also do it all in one go.

On Fri, Mar 4, 2016 at 2:59 PM Nicolas Charpentier notifications@github.com
wrote:

Should we put that into a branch named v0.9.0 waiting for the other
changes ?


Reply to this email directly or view it on GitHub
#91 (comment)
.

davidLeonardi added a commit that referenced this pull request Mar 14, 2016
include route in args for willFocus/didFocus
@davidLeonardi davidLeonardi merged commit 5f0f80f into react-native-simple-router-community:master Mar 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants