Skip to content

Conversation

mjackson
Copy link
Member

This commit also removes our dependency on when.js and introduces
a callback argument in transition hooks. Users are expected to call
callback(error) when they're finished doing async stuff. If they
don't have a callback in their argument list, we automatically call
the callback for them for backwards compat.

Fixes #273
Fixes #631

I have mixed feelings about this PR. Part of me wishes that everyone would get on the same page, realize how cool promises are, use webpack, and be happy. But hey, at least we get:

  1. smaller file size
  2. 1 less dependency
  3. easier compat with browserify

@ryanflorence
Copy link
Member

not that I dislike promises, but this is great 👍

Copy link
Member

Choose a reason for hiding this comment

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

I know 3 is the magic number, but what is this 3 all about?

Copy link
Member

Choose a reason for hiding this comment

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

oh nvm, args length, I was thinking we were collecting hooks or something ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make the comment clearer.

@ryanflorence
Copy link
Member

File size is 7.7kb now, saves us ~3k. I think that and the other things you've noted are worth this change:

transition.wait(promise());
promise().then(callback);

I can't find an objective reason the wait API is better either.

Copy link

Choose a reason for hiding this comment

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

typo

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks :)

@gaearon
Copy link
Contributor

gaearon commented Dec 31, 2014

I didn't realize we can check arity. I'm on board with this!

This commit also removes our dependency on when.js and introduces
a callback argument in transition hooks. Users are expected to call
callback(error) when they're finished doing async stuff. If they
don't have a callback in their argument list, we automatically call
the callback for them for backwards compat.

Fixes #273
Fixes #631
mjackson added a commit that referenced this pull request Dec 31, 2014
Remove transition.wait, use callbacks instead
@mjackson mjackson merged commit be624fe into master Dec 31, 2014
@mjackson
Copy link
Member Author

@rpflorence @gaearon We should cut another minor release with this change. Anything else you wanna get in?

@gaearon
Copy link
Contributor

gaearon commented Dec 31, 2014

@mjackson I'd love to see replaceRoutes:

      replaceRoutes: function (children) { 
        namedRoutes = {};
        routes = [];
        this.addRoutes(children);
      },

@ryanflorence
Copy link
Member

I'll get a release out today, it's a breaking api change so we'll be at 0.12

@mjackson mjackson deleted the yay-callbacks branch February 12, 2015 23:57
@JFusco
Copy link

JFusco commented Mar 19, 2015

So how would you handle a transition.redirect() when doing async/promise calls? Seems that the redirect gets ignored when trying to do something like this:

statics: {
    willTransitionTo: function (transition, params) {
        something.call(somePath())
            .then(function(){
                transition.redirect('accessDenied');
            });
    }
}

@mjackson Thanks for your hard work! Looking forward to future releases

@JFusco
Copy link

JFusco commented Mar 19, 2015

Sorry if I was not clear in my above question - since the removal of .wait(), what would be the equivalent to this?

statics: {
    willTransitionTo: function (transition, params, query) {
      transition.wait(
        checkAuthAndStoreItSomewhere().then(function (auth) {
          if (auth == null)
            transition.redirect('login');
        });
      );
    }
}

@gaearon
Copy link
Contributor

gaearon commented Mar 20, 2015

@JFusco

I think something like

statics: {
    willTransitionTo: function (transition, params, query, callback) {
      checkAuthAndStoreItSomewhere().then(function (auth) {
        if (auth == null)
          transition.redirect('login');
      }).then(callback);
    }
}

@JFusco
Copy link

JFusco commented Mar 20, 2015

Thanks @gaearon - this occurred to me this morning..just needed some sleep :)

I appreciate the quick response and the hard work...Looking forward to .13 release so I can fully support ES6 in React!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow proxying transition hooks to asynchronously loaded handlers Not working with browserify with the minifyify plugin active
6 participants