mojolicious / mojo.js

:unicorn: The Mojolicious real-time web framework for Node.js
https://mojojs.org
MIT License
486 stars 33 forks source link

simple fix for empty path named route case #111

Closed dmanto closed 1 year ago

dmanto commented 1 year ago

ctx.urlFor() seems to be broken for a root ("/") named route, like in:

app.get('/', ctx => ctx.render({text: 'Hello World'}).name('root-route');

ctx.urlFor('root-route') would return an empty string (''), instead of a single slash string ('/'). That would end provoking problems with tags like tags.linkTo()

You can reproduce the problem with this simple one-liner:

node examples/hello-template.js eval -v 'app.get("/").name("r") && app.newMockContext().tags.linkTo("r")'

That will give you a broken html link result of <a href></a> instead of the expected <a href="/"></a>

Please consider this PR to fix this issue, it just coerces an empty path to be "/" inside ctx._urlForPath() function.

Upon further consideration, it may be better to ensure that the path is specifically an empty string and not any falsy value. For example, path = path === '' ? '/' : path;. I would appreciate your feedback on this.

marcusramberg commented 1 year ago

Seems there are some linter failures.

dmanto commented 1 year ago

Seems there are some linter failures.

You are right, but these are already in the main branch. Thought core team was working on something else. I didn't change anything as they are unrelated to the issue and I don't know what was the intention of them.

kraih commented 1 year ago

Looks like a new eslint rule was added recently.

kraih commented 1 year ago

Fixed if you rebase.