Skip to content
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

URL helper keeps /index #192

Closed
george-oakling opened this issue May 27, 2020 · 10 comments
Closed

URL helper keeps /index #192

george-oakling opened this issue May 27, 2020 · 10 comments

Comments

@george-oakling
Copy link

george-oakling commented May 27, 2020

bug description
url helper doesn't care about the third optional param to keep/remove index from relative URL. Example: I have this structure:

/App.svelte
/pages/_layout.svelte
/pages/index.svelte
/pages/subpage/_layout.svelte
/pages/subpage/index.svelte

I am having a link <a href={$url('./', {param: xxx}, false)} on pages/subpages/index.svelte and it keeps the index in URL generated /pages/subpage/index?param=xxx regardless the third parameter is set to false or true.

The question is: How to remove it? 😄

version
Routify: 1.7 (and 1.8 beta)
Svelte: 3.22.3 (and 3.23.0)

@jakobrosenberg
Copy link
Member

Hi @george-oakling. I completely missed this issue. Sorry!

Is this still an issue?

@esgabo
Copy link

esgabo commented Jul 15, 2020

Hi,

It's still an issue for me.

console.log($url('./' + account.id));
console.log($url('./' + account.id, {}, false));
console.log($url('./' + account.id, {}, true));

All of these will return the same result: "/accounts/index/asdbasd163"

my structure is:
/pages/accounts/[id]/

having an index.svelte at both the accounts and [id] directory

version installed
@sveltech/routify@1.9.2

@jakobrosenberg
Copy link
Member

@esgabo, could you try ../ instead of ./?

@esgabo
Copy link

esgabo commented Jul 15, 2020

@esgabo, could you try ../ instead of ./?

console.log($url('../' + account.id));
console.log($url('../' + account.id, {}, false));
console.log($url('../' + account.id, {}, true));

Result:

/accounts/asdbasd165
/accounts/asdbasd165
/accounts/asdbasd165

I think it's still an issue, since I would expect the "index" when it's true.
For now, I'd rather to workaround by using the absolute path, because it looks weird to use '../' to reference the current path.
But this issue makes relative path unusable.

@jakobrosenberg
Copy link
Member

The index has to be supplied for it to have an effect. Making the /index optional allows isActive to treat /blog and /blog/index differently, where the former is active on mysite.com/blog and mysite.com/blog/post1 etc. while the latter is only active on mysite.com/blog.

Consider

  • home
  • blog
  • about

You don't want the home link to always be active, so you use /index instead of just /. For blog however, you don't want to add /index since you want it to match all nested paths.

I agree with your view on path resolution and for Routify 2 we'll align with filesystem conventions rather than URL conventions. For v1 we have to stick with the current convention to not introduce breaking changes.

@esgabo
Copy link

esgabo commented Jul 15, 2020

I see. Then it seems to work as expected for v1 and I just misunderstood how current path works on routify. (perhaps a clarification on the doc?)

Looking forward that v2. Keep the good work.

Thanks for the lesson :)

@jakobrosenberg
Copy link
Member

Thanks for the feedback. I think I can finally put this issue to rest.

@rixo
Copy link
Collaborator

rixo commented Jul 16, 2020

Since this is planned behavior for v2, could we maybe implement this behind an option in v1 so we can start writing v2 compatible code right away?

@jakobrosenberg
Copy link
Member

Brillliant idea @rixo.

@jakobrosenberg
Copy link
Member

This was added to 2.0. Hoping to release it in the very near future.

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

No branches or pull requests

4 participants