Results 1 to 8 of 8

Thread: Ext 6.6.0 Router bug. unneeded onExit call when more then 2 routes.

    Thank you for reporting this bug. We will make it our priority to review this report.
  1. #1
    Sencha Premium User
    Join Date
    Oct 2014
    Posts
    5

    Default Ext 6.6.0 Router bug. unneeded onExit call when more then 2 routes.

    Ext 6.6.0
    Ext.route.Router::doRun function
    line 213
    Code:
                     Ext.Array.remove(unmatched, route);
    
    
                        if (!matched[name]) {
                            matched[name] = 1;
                        }
                    } else if (!matched[name]) { ////////HERE
                        unmatched.push(route);
                    }
    When there is more then 2 routes, Routes duplicates in unmatched array. And Ext.Array.remove, removes only one.
    There should also be indexOf check on line 213.

  2. #2
    Sencha Premium User mitchellsimoens's Avatar
    Join Date
    Mar 2007
    Location
    Gainesville, FL
    Posts
    40,379

    Default

    I'm curious about this, can you help create a test case for this? I have this on that defines three foo routes (one in each global controller and another in the application) and a foo route in the application:



    When I run that fiddle, I get these logs:

    Fiddle.$application onFoo
    Fiddle.controller.Foo onFoo
    Fiddle.controller.Bar onFoo
    Fiddle.$application onBarExit
    So first thing I see is that onBarExit shouldn't fire. I can see what's going on there but that doesn't seem like the bug you are reporting. It needs to know if the route was active, it's just assuming it was which is bad. An override that fixes that for me is (it's the last for loop):

    Code:
    Ext.define(null, {
        override: 'Ext.route.Router',
    
        doRun : function (tokens) {
            var me = this,
                app = me.application,
                routes = me.routes,
                i = 0,
                length = tokens.length,
                matched = {},
                unmatched = [],
                token, found,
                name, route, recognize;
     
            for (; i < length; i++) {
                token = tokens[i];
                found = false;
     
                for (name in routes) {
                    route = routes[name];
                    recognize = route.recognize(token);
     
                    if (recognize) {
                        found = true;
     
                        if (recognize !== true) {
                            // The document fragment may have changed but the token 
                            // part that the route recognized did not change. Therefore 
                            // is was matched but we should not execute the route again. 
                            route
                                .execute(token, recognize)
                                .then(null, Ext.bind(me.onRouteRejection, me, [route], 0));
                        }
     
                        Ext.Array.remove(unmatched, route);
     
                        if (!matched[name]) {
                            matched[name] = 1;
                        }
                    } else if (!matched[name]) {
                        unmatched.push(route);
                    }
                }
     
                if (!found) {
                    if (app) {
                        //backwards compat 
                        app.fireEvent('unmatchedroute', token);
                    }
     
                    Ext.fireEvent('unmatchedroute', token);
                }
            }
     
            i = 0;
            length = unmatched.length;
     
            for (; i < length; i++) {
                route = unmatched[i]
                
                // check if the route has a lastToken
                // this property is cleared in it's
                // onExit method so it should be truthy
                // if the route was "active"
                if (route.lastToken) {
                    route.onExit();
                }
            }
        }
    })
    So back to your bug about Ext.Array.remove only removing one route. The issue is a route is named meaning if you have multiple routes with the same url pattern, only a single route is created and "handlers" are added for all the different places you have the url pattern. For example, I defined 3 "foo" routes in my fiddle but it only creates 1 route but that route has 3 handlers. Ext.route.Router.routes is an object that tracks these routes by name so you won't have duplicate names tracked. When a class defines it's routes and they are connected to the Router, the Router first checks to see if there is a route by that name (url pattern) and if so then it adds a handler, it doesn't create a new route each time. So if you were to run my fiddle, you'll see this log:

    ** There are 2 routes created: bar, foo
    Route "bar" has 1 handlers
    Route "foo" has 3 handlers
    In the doRun code, matched is an object that tracks matching of a route by name and unmatched is an array that has routes pushed to it. So unmatched should only get a route pushed once since there is only ever one route per name. So the Ext.Array.remove should only need to remove one route always.

    I'm not saying I don't believe you, just theoretically shouldn't happen and maybe my test case is too small.
    Mitchell Simoens @LikelyMitch

    Check out my GitHub:
    https://github.com/mitchellsimoens

    Posts are my own, not any current, past or future employer's.

  3. #3
    Sencha Premium Member richardvd's Avatar
    Join Date
    Jun 2011
    Location
    NL
    Posts
    253

    Default

    I also get a lot of spurious calls to my exit handler with the original 6.6.0 code.

    The override doesn't work well for the token at the end, it seems to happen if there are more than two tokens.

    I forked your Fiddle and added more test cases: https://fiddle.sencha.com/fiddle/2j3j/preview (click the buttons from 1 to 6 in that order and look at the console output).

    Button 1: Go to #foo/1|bar/2|baz/3
    => exit triggers on route baz (but it shouldn't because all routes are new)


    Button 2: Go to #foo/1|bar/3|baz/3 (change bar/2 to bar/3)
    => exit triggers on route baz (but it shouldn't because route baz is still active), and action triggers on route baz (wrong because baz is unchanged)


    Button 3: Go to #foo/1|baz/3 (remove bar/3)
    => exit triggers on route bar (OK), but action triggers on route baz (wrong because baz is unchanged)


    Button 4: Go to #foo/1|bar/2|baz/3 (add bar/2 in the middle)
    => action triggers on route bar (OK), but exit triggers on route baz (wrong because route baz is still active)


    Button 5: Go to #foo/1|bar/2|baz/4 (change baz/3 to baz/4)
    => action triggers on route baz (OK), but also exit triggers on route baz (wrong because route baz is still active)


    Button 6: Go to #foo/1|bar/2 (remove baz/4)
    => nothing happens (but exit should have triggered on route baz)

  4. #4
    Sencha Premium User mitchellsimoens's Avatar
    Join Date
    Mar 2007
    Location
    Gainesville, FL
    Posts
    40,379

    Default

    Ah I see, so not multiple routes, multiple tokens in the hash. Gotcha and thanks for the fork. Here is my fork with a modified override from mine before that should fix this:



    When I click on button 1, these are the logs:

    1. => exit triggers on route baz (but it shouldn't because all routes are new)
    Fiddle.$application onFoo {id: "1"}
    Fiddle.controller.Foo onFoo {id: "1"}
    Fiddle.controller.Bar onFoo {id: "1"}
    Fiddle.$application onBar {id: "2"}
    Fiddle.$application onBaz {id: "3"}
    Looks good there, no baz exit.

    Button 2 logs:

    2. => exit triggers on route baz (but it shouldn't because route baz is still active), and action triggers on route baz (wrong because baz is unchanged)
    Fiddle.$application onBar {id: "3"}
    Cool, only the bar route fired since it changed, no baz triggering.

    Button 3 logs:

    3. => exit triggers on route bar (OK), but action triggers on route baz (wrong because baz is unchanged)
    Fiddle.$application onBarExit {removedHash: "bar/3"}
    Great, only the bar exit was triggered, no extra baz executions.

    Button 4 logs:

    4. => action triggers on route bar (OK), but exit triggers on route baz (wrong because route baz is still active)
    Fiddle.$application onBar {id: "2"}
    Great, only the bar action was triggered, nothing with baz again.

    Button 5 logs:

    5. => action triggers on route baz (OK), but also exit triggers on route baz (wrong because route baz is still active)
    Fiddle.$application onBaz {id: "4"}
    Great, only the bar action was triggered, nothing with baz again.

    Button 6 logs:

    6. => nothing happens (but exit should have triggered on route baz)
    Fiddle.$application onBazExit {removedHash: "baz/4"}
    So only baz exit handler fired.

    So looks like this override did the trick. Instead of using Ext.Array.remove and an indexOf call, I swapped the array out for an object so that I could nullify the name and make that mean something. I added a comment about what undefined vs null means, basically null means it went from being unmatched to matched and we should only set it on unmatched if it's undefined.

    Code:
    Ext.define(null, {
        override: 'Ext.route.Router',
    
        doRun : function (tokens) {
            var me = this,
                app = me.application,
                routes = me.routes,
                i = 0,
                length = tokens.length,
                matched = {},
                unmatched = {},
                token, found,
                name, route, recognize;
    
            for (; i < length; i++) {
                token = tokens[i];
                found = false;
    
                for (name in routes) {
                    route = routes[name];
                    recognize = route.recognize(token);
    
                    if (recognize) {
                        found = true;
    
                        if (recognize !== true) {
                            // The document fragment may have changed but the token
                            // part that the route recognized did not change. Therefore
                            // is was matched but we should not execute the route again.
                            route
                                .execute(token, recognize)
                                .then(null, Ext.bind(me.onRouteRejection, me, [route], 0));
                        }
    
                        unmatched[name] = null
    
                        if (!matched[name]) {
                            matched[name] = 1;
                        }
                    } else if (
                        // check to see if the route has been matched yet
                        !matched[name] &&
                        // undefined = route hasn't been set or null'd
                        // null = went from being unmatched (or nothing at all) to matched
                        // route = route hasn't been matched in a token (yet)
                        !Ext.isDefined(unmatched[name])
                    ) {
                        unmatched[name] = route
                    }
                }
    
                if (!found) {
                    if (app) {
                        //backwards compat
                        app.fireEvent('unmatchedroute', token);
                    }
    
                    Ext.fireEvent('unmatchedroute', token);
                }
            }
    
            for (name in unmatched) {
                route = unmatched[name]
    
                // check if the route has a lastToken
                // this property is cleared in it's
                // onExit method so it should be truthy
                // if the route was "active"
                // note route may be null
                if (route && route.lastToken) {
                    route.onExit();
                }
            }
        }
    })
    Mitchell Simoens @LikelyMitch

    Check out my GitHub:
    https://github.com/mitchellsimoens

    Posts are my own, not any current, past or future employer's.

  5. #5
    Sencha Premium Member richardvd's Avatar
    Join Date
    Jun 2011
    Location
    NL
    Posts
    253

    Default

    Getting closer! But in my project it still fails in this case:

    There are three tokens (#foo/1|bar/2|baz/3). Remove the baz token by name. Result: nothing happens (expected result: baz exit handler fires).
    Code:
    me.redirectTo({
      'baz/:id': null
    });

  6. #6
    Sencha Premium Member richardvd's Avatar
    Join Date
    Jun 2011
    Location
    NL
    Posts
    253

    Default

    I created an override for Ext.route.Mixin.redirectTo(). When passing in an object to clear a token like I did in post #5, it cleared the lastToken for that route, but in Ext JS 6.6.0 Ext.route.Route.onExit() is responsible for that. So I removed the line containing route.lastToken = null;.

    Now with both overrides in this new Fiddle the exit handler is finally meeting my expectations! Thanks for your help!

    https://fiddle.sencha.com/fiddle/2j3p/preview



    Override 1 (from post #4):
    Code:
    Ext.define(null, {
        override: 'Ext.route.Router',
    
        doRun : function (tokens) {
            var me = this,
                app = me.application,
                routes = me.routes,
                i = 0,
                length = tokens.length,
                matched = {},
                unmatched = {},
                token, found,
                name, route, recognize;
    
            for (; i < length; i++) {
                token = tokens[i];
                found = false;
    
                for (name in routes) {
                    route = routes[name];
                    recognize = route.recognize(token);
    
                    if (recognize) {
                        found = true;
    
                        if (recognize !== true) {
                            // The document fragment may have changed but the token
                            // part that the route recognized did not change. Therefore
                            // is was matched but we should not execute the route again.
                            route
                                .execute(token, recognize)
                                .then(null, Ext.bind(me.onRouteRejection, me, [route], 0));
                        }
    
                        unmatched[name] = null;
    
                        if (!matched[name]) {
                            matched[name] = 1;
                        }
                    } else if (
                        // check to see if the route has been matched yet
                        !matched[name] &&
                        // undefined = route hasn't been set or null'd
                        // null = went from being unmatched (or nothing at all) to matched
                        // route = route hasn't been matched in a token (yet)
                        !Ext.isDefined(unmatched[name])
                    ) {
                        unmatched[name] = route;
                    }
                }
    
                if (!found) {
                    if (app) {
                        //backwards compat
                        app.fireEvent('unmatchedroute', token);
                    }
    
                    Ext.fireEvent('unmatchedroute', token);
                }
            }
    
            for (name in unmatched) {
                route = unmatched[name];
    
                // check if the route has a lastToken
                // this property is cleared in it's
                // onExit method so it should be truthy
                // if the route was "active"
                // note route may be null
                if (route && route.lastToken) {
                    route.onExit();
                }
            }
        }
    });
    Override 2 (new):
    Code:
    Ext.define(null, {
        override: 'Ext.route.Mixin',
    
        requires: [
            'Ext.route.Router',
            'Ext.util.History'
        ],
    
        redirectTo: function (hash, opt) {
            var me          = this,
                currentHash = Ext.util.History.getToken(),
                Router      = Ext.route.Router,
                delimiter   = Router.getMultipleToken(),
                tokens      = currentHash ? currentHash.split(delimiter) : [],
                length      = tokens.length,
                force, i, name, obj, route, token, match;
    
            if (hash === -1) {
                return Ext.util.History.back();
            } else if (hash === 1) {
                return Ext.util.History.forward();
            } else if (hash.isModel) {
                hash = hash.toUrl();
            } else if (Ext.isObject(hash)) {
                //Passing an object attempts to replace a token in the hash.
                for (name in hash) {
                    obj = hash[name];
    
                    if (!Ext.isObject(obj)) {
                        obj = {
                            token: obj
                        };
                    }
    
                    if (length) {
                        route = Router.getByName(name);
    
                        if (route) {
                            match = false;
    
                            for (i = 0; i < length; i++) {
                                token = tokens[i];
    
                                if (route.matcherRegex.test(token)) {
                                    match = true;
    
                                    if (obj.token) {
                                        //a token was found in the hash, replace it
                                        if (obj.fn && obj.fn.call(this, token, tokens, obj) === false) {
                                            //if the fn returned false, skip update
                                            continue;
                                        }
    
                                        tokens[i] = obj.token;
    
                                        if (obj.force) {
                                            //clear lastToken to force recognition
                                            route.lastToken = null;
                                        }
                                    } else {
                                        //remove token
                                        tokens.splice(i, 1);
    
                                        i--;
                                        length--;
    
                                        // BEGIN BUGFIX
                                        // don't reset lastToken here,
                                        // as of Ext JS 6.6.0 onExit() will take care of this
    
                                        //reset lastToken
                                        //route.lastToken = null;
    
                                        // END BUGFIX
                                    }
                                }
                            }
    
                            if (obj && obj.token && !match) {
                                //a token was not found in the hash, push to the end
                                tokens.push(obj.token);
                            }
                        }
                    } else if (obj && obj.token) {
                        //there is no current hash, push to the end
                        tokens.push(obj.token);
                    }
                }
    
                hash = tokens.join(delimiter);
            }
    
            if (opt === true) {
                //for backwards compatibility
                force = opt;
                opt = null;
            } else if (opt) {
                force = opt.force;
            }
    
            length = tokens.length;
    
            if (force && length) {
                for (i = 0; i < length; i++) {
                    token = tokens[i];
    
                    Router.clearLastTokens(token);
                }
            }
    
            if (currentHash === hash) {
                if (force) {
                    //hash won't change, trigger handling anyway
                    Router.onStateChange(hash);
                }
    
                //hash isn't going to change, return false
                return false;
            }
    
            if (opt && opt.replace) {
                Ext.util.History.replace(hash);
            } else {
                Ext.util.History.add(hash);
            }
    
            return true;
        }
    },
    function() {
        // auto override this mixin in already existing classes
        var me = this,
            reservedPropertiesRe = [
                '^\\$.*', '^_.*', 'afterClassMixedIn', 'alias', 'alternateClassName',
                '^apply.*', 'cachedConfig', 'config', 'defaultConfig', '^destroy.*',
                'extend', '^get.*', 'inheritableStatics', 'mixinConfig', 'mixinId',
                'mixins', 'override', 'onClassMixedIn', 'platformConfig', 'privates',
                'requires', 'self', '^set.*', 'singleton', 'statics', 'superclass',
                '^update.*', 'uses', 'xtype'
            ],
            re = new RegExp(reservedPropertiesRe.join("|"));
    
        Ext.Object.each(Ext.ClassManager.classes, function(name, targetClass) {
            if (targetClass.prototype && targetClass.prototype.mixins) {
                Ext.Object.each(targetClass.prototype.mixins, function(id, targetMixinClass) {
                    var mixinClassName = targetMixinClass.$className,
                        overrideClassName = me.prototype.$className;
    
                    if (mixinClassName===overrideClassName) {
                        Ext.Object.each(me.prototype, function(propertyName, newClass) {
                            if ( !re.test(propertyName)
                                && ( newClass!==targetClass.prototype[propertyName] )) {
                                targetClass.prototype[propertyName] = newClass;
                            }
                        });
                    }
                });
            }
        }, this);
    });

  7. #7
    Sencha Premium User
    Join Date
    Oct 2014
    Posts
    5

    Default

    Hello gentlemen.
    Thanks for your help, Im still think that my suggested fix in original post is enough to make everything work.

    However, one not obvious problem can appear since we have Route.name property.
    So this code is not working as it should anyway:
    controller1:
    Code:
    routes:{
        'baz/:baz' : {
             name: FOO
         }
    }
    controller2:
    Code:
    routes:{
        'foo/:foo' : {
             name: FOO
         }
    }
    Because routes collected by name. So there is 1 FOO route, with 2 handlers but 1 regex parser.
    Hopefully nobody will hit that.

  8. #8
    Sencha Premium User mitchellsimoens's Avatar
    Join Date
    Mar 2007
    Location
    Gainesville, FL
    Posts
    40,379

    Default

    Yes, you can have naming conflicts and that's one of those "features" that may look like a bug but isn't. The reason they are named now is sort of shown in Richard's example where he uses an object when executing redirectTo:

    Code:
    me.redirectTo({
      'baz/:id': null
    })
    Of course your route hash could be much bigger and more complex but you may also have those redirectTos spread around but what if you changed the url pattern to be 'baz/:id/omething' now you have to go around your app and update all your redirectTos which is error prone from missing or typos. So you can have names:

    Code:
    routes: {
      'baz/:id/:something': {
        action: 'onBaz',
        name: 'baz'
      }
    }
    
    this.redirectTo({
        baz: 'baz/1/2'
    })
    Hopefully when you start giving routes names, you understand what's going on more but I do wish I had access to update docs/guides to show lots of the new stuff and deeper dives.
    Mitchell Simoens @LikelyMitch

    Check out my GitHub:
    https://github.com/mitchellsimoens

    Posts are my own, not any current, past or future employer's.

Similar Threads

  1. [INFOREQ] Router - multi-token routes should execute in order
    By LesJ in forum Ext JS 6.x Bugs
    Replies: 12
    Last Post: 8 Aug 2017, 6:07 AM
  2. Router - using multiple routes
    By LesJ in forum Ext JS 6.x Q&A
    Replies: 1
    Last Post: 4 Jun 2017, 8:38 AM
  3. Replies: 9
    Last Post: 26 Sep 2016, 6:02 PM
  4. Replies: 1
    Last Post: 20 Mar 2015, 11:40 AM

Tags for this Thread

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •