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

Translated paths #92

Merged
merged 10 commits into from
Sep 23, 2023
Merged

Translated paths #92

merged 10 commits into from
Sep 23, 2023

Conversation

claudioshiver
Copy link

Use the new Qwik City rewriteRoutes option to translate complete paths.

This PR add the useTranslatePath hook that can translate the application routes.

Now you can translate http://127.0.0.1:4173/page to http://127.0.0.1:4173/it-IT/pagina instead of http://127.0.0.1:4173/it-IT/page that will not be available anymore.

@robisim74
Copy link
Owner

Hi @claudioshiver,

thanks a lot for this pr! I'll look at it as soon as possible.

@robisim74
Copy link
Owner

Hi Claudio,

There were no requests for this feature in this repo, so what follows is just my opinion as a maintainer.

First we should leave the old tutorial-routing.md doc file and create a new one for the translated router (something like tutorial-translated-routing.md), for two reasons:

  • those who were using the native file-based routing method may not want or need to switch to rewrite-based routing
  • this way I will be able to merge the pr without publishing immediately (the documentation is generated automatically). I'll update the summaries and links to the new doc after publishing the library

The rewrite-based method seems to work well in basic cases:

  • dev mode
  • prod mode
  • SSG
  • params & search params in paths

Ok, now as a developer I have tried some configurations (operating on the rewriteRoutes):

  • A type for rewriteRoutes would be useful
  • The paths property appear to be fragments of a path: or can you put complete paths? It is not clear
  • prefix would seem to be optional: but if I try to remove it from the configuration (to have only translated urls without language parameter) it doesn't seem to work (this is useful if you have a domain for each locale)
  • How to manage only the prefix without translating any path? I tried removing the paths but it doesn't seem to work. And should the prefix mapping still be provided anyway? This would be redundant compared to current file-based routing
  • Last but not least: how to handle non allowed characters in URLs? and especially ideogram-based languages such as Chinese, Japanese etc.? As far as I know, they should be converted to Punycode: but does Qwik support this?

Let me know what you think.

@claudioshiver
Copy link
Author

Hi Roberto,
in general you're right, there are basically two different approaches, the first with prefix only (actual example with route parameter [...lang]) and the other with prefix and url rewriting (my new approach).

These two approaches can't work together, i'll explain in the following points.

I reverted back the tutorial-routing.md so who is using the current approach still have a reference.
I've created the new tutorial-routing-rewrite.md so who want can chose this new approach.

About the other points:

  • Unfortunately i forgot to export the rewriteRoutes type directly but i replaced the type in qwik-speak with QwikCityVitePluginOptions['rewriteRoutes'].
  • Yes as you said the paths are in reality fragments but i didn't know how to name them. paths seemed more friendly to me but anyway you're right, it could be a little bit misleading. Anyway the behavior is documented here: https://qwik.builder.io/docs/routing/#rewrite-routes
  • Yes the prefix is optional but if you remove it probably you need a new way to determine the locale, maybe from the browser agent or from a request header, so you should then change the onRequest function in the plugin.ts file according to your needs.
  • If you want to manage only the prefix without translating paths you should follow the current approach with route param [...lang] without using the rewriteRoutes at all.
  • About punycode i think it's not readable both for chinese/japanese and for europeans/us users. I think the best approach for them would be to use unicode characters. There are such language variations for Japanese (Romaji) and for Chinese (Pinyin) and i think for other languages as well.

Let me know if it's all clear.

What should i do with the base app example, i changed everything to work with the new approach. Do you prefer to rollback that part and just describe the new possible approach in tutorial-routing-rewrite.md ?

Thank you.

@robisim74
Copy link
Owner

Currently, non-ASCII characters are not supported by @claudioshiver solution, therefore even a simple path like "città" in Italian doesn't work.

Path encoding/decoding should be added. For example if I do this:

export const rewriteRoutes = [
  {
    prefix: 'ja-JP',
    paths: {
      'page': encodeURIComponent('ページ')
    }
  }, {
    prefix: 'pt-PT',
    paths: {
      'page': encodeURIComponent('página')
    }
  }
]

(just an example, encoding should be done internally) and I decode the return value of rewritePath:

return decodeURIComponent(translated.join('/')) // decode

it works. Ok, then I think the developer should also configure the web server to handle non-ASCII characters in URLs.

But I'm not sure this is enough. We should look into it further.

@ahnpnl , @mahmoudajawad any opinion on how to work with paths translated into non-Latin based languages?

@claudioshiver
Copy link
Author

My opinion is that there is no reason to use non ascii characters in urls.. Even the italian "città" for example in the URLs is usually written as "citta" without any accent and as far as i know even japanese or chinese URLs doesn't usually use idiograms but latin letters with their phonetic language alternative.

Anyway i think this is just an edge case, everything should work for other cases except for special characters in URLs and as you said it can be done at config time encoding the URLs if you want.

@robisim74 robisim74 merged commit 71187a3 into robisim74:main Sep 23, 2023
2 checks passed
@robisim74
Copy link
Owner

Awesome! Thanks a lot!

Well be released soon.

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

Successfully merging this pull request may close these issues.

2 participants