Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix issues with incorrect current result for pure param routes #377

Merged
merged 3 commits into from
Nov 26, 2020
Merged

Fix issues with incorrect current result for pure param routes #377

merged 3 commits into from
Nov 26, 2020

Conversation

TomHart
Copy link
Contributor

@TomHart TomHart commented Nov 26, 2020

route().current('page', {page: 'some-path'}) returns true when on /, this fixes that.

I discovered this in my project, where I have the routes

Route::get('/', [PageController::class, 'home'])->name('home');
Route::get('/{page}', [PageController::class, 'show'])->name('page');

And in my nav :active=route().current('page', 'about') was returning true when I was on the home page

Swapping current in dist/index.js for this verified the fix works in my project too

current(t, e) {
            const r = this.t.absolute ? window.location.host + window.location.pathname : window.location.pathname.replace(this.t.url.replace(/^\w*:\/\/[^/]+/, ""), "").replace(/^\/+/, "/"), [n, i] = Object.entries(this.t.routes).find(([e, n]) => new N(t, n, this.t).matchesUrl(r)) || [void 0, void 0];
            if (!t) return n;
            const o = new RegExp("^" + t.replace(".", "\\.").replace("*", ".*") + "$").test(n);
            if(e){
                e = this.s(e, new N(n, i, this.t))
                var z = Object.entries(this.l(i));
                if(!z.length) return false;
                return z.filter(([t]) => e.hasOwnProperty(t)).every(([t, r]) => e[t] == r)
            }
            return o
        }

@bakerkretzmar
Copy link
Collaborator

@TomHart thanks a lot for this! In investigating it some more I realized that the example you shared will actually return true for any route name you pass it, if you also pass it any params... I'm going to try to see if fixing that issue fixes this one too, but either way will fix asap.

@bakerkretzmar bakerkretzmar merged commit 65a5deb into tighten:main Nov 26, 2020
@bakerkretzmar
Copy link
Collaborator

@TomHart just want to add thanks in particular for including tests! They were super helpful for investigating this and led me to find the other bug at play here.

For my own future reference, and anyone else who ends up here—the root cause of this issue is that JavaScript's built-in every() Array method returns true on empty arrays. Because we're calling every() on the array of parameters in the URL, not the ones passed to the current() method, we would almost always get true if the URL itself doesn't have parameters, because the equality check just doesn't happen at all.

@TomHart
Copy link
Contributor Author

TomHart commented Nov 26, 2020

You're welcome :). I did notice a few other errors whilst trying to investigate this, with the route variable being passed to _parse, and it throwing errors when it tries to get .bindings from it, but I couldn't reproduce it once I started playing in code, if I figure it out I'll make a new PR to test/fix it.

Any timeframe btw when the next version will be released with this fix in it?

@bakerkretzmar
Copy link
Collaborator

Hopefully today but not sure, otherwise end of next week at the latest. The bindings errors are from passing current() a route name that doesn't exist, which is one of the other bugs I'm working on now 😄

@TomHart
Copy link
Contributor Author

TomHart commented Nov 26, 2020

Sounds good! Was just curious :D

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants