Teach named route function to handle glob routes #3702
Conversation
| @@ -250,8 +254,14 @@ describe('replaceParams', () => { | |||
| '/boolean/false' | |||
| ) | |||
|
|
|||
| expect(replaceParams('/undef/{undef}', { undef: undefined })).toEqual( | |||
| '/undef/undefined' | |||
| expect(() => replaceParams('/undef/{undef}', { undef: undefined })).toThrowError( | |||
There was a problem hiding this comment.
I added this testcase to allow/properly handle values explicitly set to undefined like this: { undef: undefined } (as opposed to not having them at all: { } or { und: 'wrong key name' }. Your changes breaks this behavior (as seen by the need to update the expected result)
There was a problem hiding this comment.
Ah, interesting. What's the use case for that? Seems like a weird thing to do, to want "undefined" printed in the URL like that.
There was a problem hiding this comment.
Yes, I agree it's weird. "But why limit what the users can do if we don't have to?" That was pretty much my reasoning when writing the code and test. I don't need the feature/behavior myself. So if you do have a good reason to limit our users I'll happily accept the changed test expectations.
There was a problem hiding this comment.
The code also handles false (and true) by printing them in the url, which are also weird things to have in the url. Why should we treat an explicit value of undefined differently than false in this case?
There was a problem hiding this comment.
undefined to me always feels like "pretend this variable doesn't exist", which in this context would be like un-setting it in the object, and would result in the same thing as if you didn't pass it at all.
The boolean param type is perhaps a bit weird, but only true or false will ever be allowed or printed. I would never expect that passing an undefined parameter would result in a literal undefined to be printed in the URL.
Another way of thinking about it is as a round trip from Router intake to named route function output. As a user, I would expect that whatever I send into the named route function will result in a link that produces the same inputs I receive from the router on the other end. In the case of undefined I give the named route function an undefined value and it gives me back a string.
There was a problem hiding this comment.
"pretend this variable doesn't exist", which in this context would be like un-setting it in the object
That's what delete is for 🙂
Another way of thinking about it...
For undefined to make sense for the user they'd have to write their own paramTypes that transforms the string.
"I give the named route function a Date value and it gives me back a string.". Yeah, because you didn't tell the router how to transform date-strings to Date.
|
As I said when we chatted about his, I think we should only throw an error in dev, not in prod. And I think @orta and @peterp agrees according to their input in the chat. Either by having an Could you also please see if you could fix the linting errors? Thanks! |
I'm not 100% on board with this yet, so I'd like to split that change in behavior into its own PR, so I've made this backwards compatible (with caveat on |
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
…est-fun * 'main' of github.com:redwoodjs/redwood: Throw error on missing route param. (redwoodjs#3714) Regenerate lock file(s) (redwoodjs#3722) update yarn.lock udpate yarn.lock v0.38.1 update yarn.lock Have Redwood figure out in what order tables need to be cleared during scenario teardown (redwoodjs#3716) Base64 encode smaller required images during prerender (redwoodjs#3705) Have Redwood figure out in what order tables need to be cleared during scenario teardown (redwoodjs#3716) Base64 encode smaller required images during prerender (redwoodjs#3705) Fixes syntax error Firebase Auth template initializeApp (redwoodjs#3709) Fixes UserInputError mesage display in FormError (redwoodjs#3704) dbAuth: Resolved typos and inconsistencies in templates (redwoodjs#3698) fixed styling post-setup-tailwind command (redwoodjs#3696) More router tests. (redwoodjs#3718) Fix Fastify link. Remove Codesee integration. (redwoodjs#3717) Teach named route function to handle glob routes (redwoodjs#3702)
Named route functions currently don't work for glob routes. This PR will properly handle glob routes and params on non-slash boundaries.
This PR also makes the named route function throw an error if any route params are missing.