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

[WIP] Fix #59 - Improved Middlewares #127

Closed
wants to merge 1 commit into
base: improved-middlewares
from

Conversation

Projects
None yet
2 participants
@delgermurun
Contributor

delgermurun commented May 27, 2015

Add support for #59

@delgermurun delgermurun force-pushed the delgermurun:improved-middlewares branch from ff5a803 to b16cd1c May 27, 2015

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented May 27, 2015

I've made quick middleware => triggersEnter change.

@arunoda am I heading right direction?

_.each(self._middleware, function(fn) {
self._page("*", fn);
self._page('*', function(ctx, next) {

This comment has been minimized.

@arunoda

arunoda May 27, 2015

Contributor

Any reason for running this in a different page middleware and differ it?

This comment has been minimized.

@delgermurun

delgermurun May 27, 2015

Contributor

There is no reason, just experiment. Moved to initialize.

This comment has been minimized.

@arunoda

arunoda May 27, 2015

Contributor

Okay :)

@arunoda

This comment has been minimized.

Contributor

arunoda commented May 27, 2015

Yep. You are on the right track :)
For now, we need to keep the middleware API as it is until we release 2.0

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented May 28, 2015

@arunoda I've just added exit triggers support. Can you quickly glance through it? Thanks.

self._page("*", function(ctx, next) {
_.each(self._triggersEnter, function(fn) {
if (typeof fn === 'function') {
fn(ctx);

This comment has been minimized.

@arunoda

arunoda May 28, 2015

Contributor

This ctx should be ._current.
It should be the same for others as well.

This comment has been minimized.

@delgermurun

delgermurun May 28, 2015

Contributor

Do you mean calling every triggers with self._current like fn(self._current)?

This comment has been minimized.

@arunoda

arunoda May 28, 2015

Contributor

Yes.

On Thu, May 28, 2015 at 2:45 PM Delgermurun Purevkhuu <
notifications@github.com> wrote:

In client/router.js
#127 (comment)
:

self._page.callbacks = [];

  • self._page.exits = [];
  • // add global enter triggers
  • self._page("*", function(ctx, next) {
  • _.each(self._triggersEnter, function(fn) {
  •  if (typeof fn === 'function') {
    
  •    fn(ctx);
    

Do you mean calling every triggers with self._current like
fn(self._current)?


Reply to this email directly or view it on GitHub
https://github.com/meteorhacks/flow-router/pull/127/files#r31215724.

This comment has been minimized.

@delgermurun

delgermurun May 28, 2015

Contributor

But _current is empty ({}) on global triggersEnter?

This comment has been minimized.

@arunoda

arunoda May 28, 2015

Contributor

What if we call triggersEnter inside our main tracker rather than, doing it inside `page(*)
see: https://github.com/delgermurun/flow-router/blob/improved-middlewares/client/router.js#L313

});
// add global exit triggers
self._page.exit("*", function(ctx, next) {

This comment has been minimized.

@arunoda

arunoda May 28, 2015

Contributor

updateCallback function seems pretty lengthy now. How abotu moving breaking these into multiple functions. We can write it like this.

...

self._page("*", self._processTriggersBefore.bind(self))
self._page.exit("*", self._processTriggersExit.bind(self))

...
@delgermurun

This comment has been minimized.

Contributor

delgermurun commented May 28, 2015

@arunoda updated.

_.each(self._middleware, function(fn) {
self._page("*", fn);
});
_.each(self._routes, function(route) {
self._page(route.path, route._handler);

This comment has been minimized.

@arunoda

arunoda May 28, 2015

Contributor

May be we can isolate this into a seperate function.

@arunoda

This comment has been minimized.

Contributor

arunoda commented May 28, 2015

Overall this looks great to me.
But I didnt try it out locally.
Let's have some tests too.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented May 28, 2015

Today I'll implement only and except. Then I'm going to add tests.

@arunoda

This comment has been minimized.

Contributor

arunoda commented May 28, 2015

Superb.
On 2015 මැයි 29, සිකු at පෙ.ව. 4.20 Delgermurun Purevkhuu <
notifications@github.com> wrote:

Today I'll implement only and except. Then I'm going to add tests.


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

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented May 29, 2015

@arunoda only and except support added. Please check this out.

P.S: Sorry for interrupting too much :)

return false;
}
if (!name) {

This comment has been minimized.

@arunoda

arunoda May 29, 2015

Contributor

This one affect to the only logic. So, even if it's only for one route
It's possible to call unnamed routes.

This comment has been minimized.

@delgermurun

delgermurun May 29, 2015

Contributor

How should we declare only for this case? only: [''], only:[] or something else?

This comment has been minimized.

@delgermurun

delgermurun May 29, 2015

Contributor

Oh, I understand incorrectly. Ignore above comment.

I just realized there is bug, sorry.

This comment has been minimized.

@arunoda

arunoda May 29, 2015

Contributor

:)

return true;
}
if (!!fn._only && _.indexOf(fn._only, name) === -1) {

This comment has been minimized.

@arunoda

arunoda May 29, 2015

Contributor

It's better if we can create a map based on only and except routes
at the beginning.
Then we don't need to use indexOf.
Not only about a performance issue. It's easy to read without it.

This comment has been minimized.

@delgermurun

delgermurun May 29, 2015

Contributor

created maps

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented May 29, 2015

Does !! need on if statement (maybe for performance)? I guess I've read something about it, but can't remember exactly.

It looks so ugly.

@arunoda

This comment has been minimized.

Contributor

arunoda commented May 29, 2015

I don't think so. For if statements it's okay.
You may need to do it if you really need to send a boolean

@@ -369,3 +427,56 @@ Router.prototype._updateCallbacks = function () {
Router.prototype._page = page;
Router.prototype._qs = qs;
function shouldCallTrigger(current, fn) {

This comment has been minimized.

@arunoda

arunoda May 29, 2015

Contributor

You can create these as methods in the Router class. Then we can test them individually.

This comment has been minimized.

@delgermurun

delgermurun May 29, 2015

Contributor

I did that because of I don't want to make bigger Router class. How does it affect performance?

Don't we have other way to test :)?

This comment has been minimized.

@arunoda

arunoda May 29, 2015

Contributor

We've other ways to test.
If you feel router getting bigger, try to move them to a util namespace.
I really don't like to have private methods hidden inside like this.
(I sometimes do it although if I were lazy)

return true;
}
function getRegisterTriggersFn(triggers) {

This comment has been minimized.

@arunoda

arunoda May 29, 2015

Contributor

Same as above

@delgermurun delgermurun force-pushed the delgermurun:improved-middlewares branch from 94ea27d to 7c2b6c6 May 29, 2015

return false;
}
if (!fn._only && !name) {

This comment has been minimized.

@arunoda

arunoda May 29, 2015

Contributor

How about if we branch these multiples ifs into three.

if(fn._only) {
  // ...
} else if(fn._except) {
  // ...
} else {
  // ...
}

For my small brain, it's super hard to read this logic :)

This comment has been minimized.

@delgermurun

delgermurun May 29, 2015

Contributor

good idea :)

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented May 29, 2015

@arunoda updated.

Regarding class size, yeah it feels bigger. But let's just create method. Later we can improve.

@arunoda

This comment has been minimized.

Contributor

arunoda commented Jun 2, 2015

This implementation looks good to me. I didn't catch your last comment. Sorry for that.
When we can see tests :)
I'm eager to push this into a release.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Jun 2, 2015

@arunoda no problem, I'm glad that you liked it :D.

I'm going to push tests tomorrow.

@arunoda

This comment has been minimized.

Contributor

arunoda commented Jun 2, 2015

Awesome :)

On Tue, Jun 2, 2015 at 4:19 PM Delgermurun Purevkhuu <
notifications@github.com> wrote:

@arunoda https://github.com/arunoda no problem, I'm glad that you liked
it :D.

I'm going to push tests tomorrow.


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

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Jun 2, 2015

@arunoda added tests.

@arunoda

This comment has been minimized.

Contributor

arunoda commented Jun 3, 2015

I think test cases covers all the cases (or at least most of the cases)
I'll do a full code review again and get this PR.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Jun 3, 2015

Awesome.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Jun 3, 2015

Oh. Should I rebase it? @arunoda

@arunoda

This comment has been minimized.

Contributor

arunoda commented Jun 3, 2015

I don't think there are any conflicting changes. If you like, rebase squash it.

@delgermurun delgermurun force-pushed the delgermurun:improved-middlewares branch from a6f2edc to 52e0867 Jun 3, 2015

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Jun 3, 2015

I mean "sqaushing" and sqaushed :D

@arunoda

This comment has been minimized.

Contributor

arunoda commented Jun 3, 2015

Yeah :)
On 2015 ජූනි 3, බදාදා at පෙ.ව. 11.26 Delgermurun Purevkhuu <
notifications@github.com> wrote:

I mean "sqaushing" and sqaushed :D


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

next();
}, 0);
this._triggersEnter.push(function(ctx) {
var str = location.search.slice(1);

This comment has been minimized.

@arunoda

arunoda Jun 4, 2015

Contributor

I think here we can't depend on the location. I think right now, location is not yet changed?
Quite not sure, I'll check.

Edit: I think, I might be wrong, since now we are not depends on pagejs middleware for this

self._page.exits = [];
self._page("*", function(ctx, next) {
var str = location.search.slice(1);

This comment has been minimized.

@arunoda

arunoda Jun 4, 2015

Contributor

Why two places processing qs?

This comment has been minimized.

@delgermurun

delgermurun Jun 4, 2015

Contributor

Oh, I forgot to delete it.

_.each(self._middleware, function(fn) {
self._page("*", fn);
});
_.each(self._routes, function(route) {
self._page(route.path, route._handler);
self._processRouteTriggersExit(route);

This comment has been minimized.

@arunoda

arunoda Jun 4, 2015

Contributor

I think this function should be _registerRouteTriggersExit

@arunoda

This comment has been minimized.

Contributor

arunoda commented Jun 4, 2015

@delgermurun don't do any changes I requested. I simply did them :)

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Jun 4, 2015

@arunoda okay. Thanks.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Jun 4, 2015

@arunoda Oh, one thing to mention. How did you refactor ctx.params.query = self._qs.parse(str); section. It inserts on last of the global enter triggers. I think we should move it to contructor function.

@arunoda

This comment has been minimized.

Contributor

arunoda commented Jun 4, 2015

I did this in place where we set _current. here: https://goo.gl/jrqwgi

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Jun 4, 2015

Okay. You already noticed that :).

@arunoda

This comment has been minimized.

Contributor

arunoda commented Jun 4, 2015

I manually took the PR. It's alive.

@arunoda arunoda closed this Jun 4, 2015

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Jun 4, 2015

🎉

@delgermurun delgermurun deleted the delgermurun:improved-middlewares branch Jun 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment