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

[Feature request] Domain-based routing #99

Closed
fprl opened this issue Nov 28, 2023 · 18 comments
Closed

[Feature request] Domain-based routing #99

fprl opened this issue Nov 28, 2023 · 18 comments
Labels
enhancement New feature or request

Comments

@fprl
Copy link

fprl commented Nov 28, 2023

Hi there,

Thank you for your work, this library is amazing.

I was wondering if Domain-based routing is something you are interested in support.

Example.

Thank you.

@robisim74
Copy link
Owner

Hi,

at the moment this library provides only two helper methods, useful for the localization of the path:

  • localizePath (to add lang prefix in paths)
  • translatePath (to add lang prefix in paths and translate segments)

Implementing domain management is not difficult: just create a helper method that returns the domain based on the language. And resolve the locale in the plugin.ts file based on the domain.

In any case, we could add the support in the library:

  • in SpeakLocale we could add the default domain for the language
  • localizePath and translatePath may return the URL with the new domain (when changing language)

I have two doubts:

  • keep don't keep prefix (make it optional?)
  • in localizedPath I could make the prefix optional, but translatePath relies on Qwik's rewrite routes, and I don't know if that's possible @claudioshiver

Then in dev mode I think it should use prefixing.

Opinions?

@robisim74 robisim74 added the enhancement New feature or request label Nov 28, 2023
@fprl
Copy link
Author

fprl commented Nov 28, 2023

Thanks for taking the time to answer me,

Implementing domain management is not difficult: just create a helper method that returns the domain based on the language. And resolve the locale in the plugin.ts file based on the domain.

Agree here, the library gives you everything you need to solve this.

I have two doubts:
keep don't keep prefix (make it optional?)
in localizedPath I could make the prefix optional, but translatePath relies on Qwik's rewrite routes, and I don't know if that's possible @claudioshiver

  • Maybe I didn't understand this but — following with the idea of default domain — if you have defaultDomain and supportedDomains properties in the speak-config, it may be a good idea to remove the prefix if supportedDomains.length > 1?

Other thing that comes to my mind is that if you declare routes without prefix, devs will need to handle redirects if language doesn't match. For example:

// default domain: dutch.nl
// supported domains: spanish.es | english.com

[
  { paths: { 'huis-ibiza': 'casa-ibiza' } }, // spanish
  { paths: { 'huis-ibiza': 'villa-ibiza' } }, // english
]

If a user goes to dutch.nl/villa-ibiza it will probably work, right? I guess you can add some logic to plugin/middleware and use localizePath and translatePath to see if route exists and redirect/404?

@robisim74
Copy link
Owner

If a user goes to dutch.nl/villa-ibiza it will probably work, right?

Yes, it should work, but we would lose the mapping with the language to know which URLs are valid (I'll have to do some testing, maybe we could handle it by using an optional lang parameter in the rewrite routes instead of prefix)

So, let's imagine the expected behavior for both methods (localizePath and translatePath):

- Dev mode (prefix = true)

http://localhost:5173/
http://localhost:5173/it
http://localhost:5173/page
http://localhost:5173/it/page or http://localhost:5173/it/pagina

- Prod mode (optional prefix = true)
(it's ugly in my opinion, but it might be the only way for those who want to use SSG too)

https://example.com/
https://example.it/it
https://example.com/page
https://example.it/it/page or https://example.it/it/pagina

- Prod mode (optional prefix = false)

https://example.com/
https://example.it/
https://example.com/page
https://example.it/page or https://example.it/pagina

If a lang have no domain, we could use default domain with prefix:

https://example.com/de
https://example.com/de/page or https://example.com/de/seite

I was thinking of a configuration like this:

export const config: SpeakConfig = {
  defaultLocale: { domain: 'example.com' lang: 'en' },
  supportedLocales: [
    { domain: 'example.com', lang: 'en' },
    { domain: 'example.it', lang: 'it'  },
    { lang: 'de' }
  ],
  assets: [
    'app'
  ],
  runtimeAssets: [
    'runtime'
  ]
};

What do you think?

@fprl
Copy link
Author

fprl commented Nov 28, 2023

Yes, it should work, but we would lose the mapping with the language to know which URLs are valid (I'll have to do some testing, maybe we could handle it by using an optional lang parameter in the rewrite routes instead of prefix)

Yes, this was what I was trying to say haha.

- Dev mode (prefix = true)
Perfect!

- Prod mode (optional prefix = true)
If this would be my choice I will go with a SSR only feature, same as the Astro team.

Why? You can solve this in many different ways:

  • Multiple Builds for Different Domains/Languages.
  • Using redirects and rewrites (though it could get messy).
  • Path-Based routing (what you wrote).
  • Middlewares provided by hosting platform (vercel, netlify, cloudflare, etc)
  • Client side redirect: I don't know if this is possible but can the user access localizePath and translatePath in SSG client side? If that's the case, they could inject a script to check things in root.tsx and redirect.

- Prod mode (optional prefix = false)
Perfect!

I was thinking of a configuration like this:

The config file looks perfect and really close to what you have today.

@robisim74
Copy link
Owner

Hi @fprl,

the feature is available in the latest release: https://github.com/robisim74/qwik-speak/releases/tag/v0.18.0

Let me know.

@fprl
Copy link
Author

fprl commented Dec 7, 2023

Hi @robisim74 , thanks a lot for pushing this feature. I'm trying to add it to my project but I get an error:

failed to load config from /project/vite.config.ts
error when starting dev server:
TypeError: Cannot read properties of undefined (reading 'map')
    at toPrefixAsNeeded (/project/node_modules/qwik-speak/lib/index.qwik.cjs:465:32)
    at Object.<anonymous> (/project/vite.config.ts:66:57)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at _require.extensions.<computed> [as .js] (file:///project/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:66340:24)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:121:18)
    at loadConfigFromBundledFile (file:///project/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:66348:21)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

I follow the Domain-based routing section in the docs, and there is a thing that I don't know if its a typo or you forgot to implement it; In the vite.config.ts file, we need to use toPrefixAsNeeded function and you pass mode as a second argument but it only takes one, rewriteRoutes.

import { qwikSpeakInline, toPrefixAsNeeded } from 'qwik-speak/inline';

import { rewriteRoutes } from './src/speak-routes';

export default defineConfig(({ mode }) => {
  return {
    plugins: [
      qwikCity({ rewriteRoutes: toPrefixAsNeeded(rewriteRoutes, mode) }), 
      /*  */
    ],
  };
});

Do you think this is related?

Context

speak-config.ts

export const config: SpeakConfig = {
  rewriteRoutes: toPrefixAsNeeded(rewriteRoutes),
  defaultLocale: { lang: "nl", currency: "EUR", timeZone: "Europe/Amsterdam" },
  supportedLocales: [
    { lang: "nl", currency: "EUR", timeZone: "Europe/Amsterdam" },
    { lang: "en", currency: "GBP", timeZone: "Europe/London" },
    { lang: "de", currency: "EUR", timeZone: "Europe/Berlin" },
    { lang: "fr", currency: "EUR", timeZone: "Europe/Paris" },
    { lang: 'it', currency: 'EUR', timeZone: 'Europe/Rome' },
    // { lang: 'en-US', currency: 'USD', timeZone: 'America/Los_Angeles' }
  ],
  domainBasedRouting: { prefix: 'as-needed' },
}

speak-routes.ts

import type { RewriteRouteOption } from 'qwik-speak';

import { domains } from './speak-config';

// Translation paths
export const rewriteRoutes: RewriteRouteOption[] = [
  { domain: domains.nl, paths: { 'huis-ibiza': 'huis-ibiza' } },
  { domain: domains.en, paths: { 'huis-ibiza': 'villa-ibiza' } },
  { domain: domains.de, paths: { 'huis-ibiza': 'villa-ibiza' } },
  { domain: domains.fr, paths: { 'huis-ibiza': 'villa-ibiza' } },
  { domain: domains.it, paths: { 'huis-ibiza': 'villa-ibiza' } },
]

plugin.ts

Plugin.ts is a copy of the docs file.

@robisim74
Copy link
Owner

robisim74 commented Dec 7, 2023

As in the code you posted, in vite.config.ts you have to import toPrefixAsNeeded from 'qwik-speak/inline'

It has a different import, because in the Vite configuration file it is executed during compilation.

@robisim74
Copy link
Owner

Then in the rewriteRoutes you have to provide prefix (as per docs): it will be removed from the toPrefixAsNeeded method in production

@fprl
Copy link
Author

fprl commented Dec 7, 2023

Good catch. Though I just updated everything and have the same issue:

failed to load config from /project/vite.config.ts
error when starting dev server:
TypeError: Cannot read properties of undefined (reading 'map')

vite.config.ts

import { defineConfig } from "vite";
import { qwikVite } from "@builder.io/qwik/optimizer";
import { qwikCity } from "@builder.io/qwik-city/vite";
import { qwikSpeakInline, toPrefixAsNeeded } from 'qwik-speak/inline';
import tsconfigPaths from "vite-tsconfig-paths";

import { rewriteRoutes } from "./src/i18n/speak-routes";

export default defineConfig(({ mode }) => {
  return {
    plugins: [
      qwikCity({ rewriteRoutes: toPrefixAsNeeded(rewriteRoutes, mode), trailingSlash: false }),
      qwikVite(),
      qwikSpeakInline({
        defaultLang: 'nl',
        supportedLangs: ['nl', 'en', 'de', 'fr', 'it'],
        assetsPath: './src/i18n'
      }),
      tsconfigPaths(),
    ],
});

speak-config.ts

import { type SpeakConfig, toPrefixAsNeeded } from 'qwik-speak';

import { rewriteRoutes } from './speak-routes';

export const config: SpeakConfig = {
  rewriteRoutes: toPrefixAsNeeded(rewriteRoutes),
  defaultLocale: { lang: "nl", currency: "EUR", timeZone: "Europe/Amsterdam" },
  supportedLocales: [
    { lang: "nl", currency: "EUR", timeZone: "Europe/Amsterdam" },
    { lang: "en", currency: "GBP", timeZone: "Europe/London" },
    { lang: "de", currency: "EUR", timeZone: "Europe/Berlin" },
    { lang: "fr", currency: "EUR", timeZone: "Europe/Paris" },
    { lang: 'it', currency: 'EUR', timeZone: 'Europe/Rome' },
    // { lang: 'en-US', currency: 'USD', timeZone: 'America/Los_Angeles' }
  ],
  domainBasedRouting: { prefix: 'as-needed' },
};

speak-routes.ts

import type { RewriteRouteOption } from 'qwik-speak';

import { domains } from './speak-config';

// Translation paths
export const rewriteRoutes: RewriteRouteOption[] = [
  { domain: domains.nl, paths: {} },
  { domain: domains.en, prefix: 'en', paths: { 'huis-ibiza': 'villa-ibiza' } },
  { domain: domains.de, prefix: 'de', paths: { 'huis-ibiza': 'villa-ibiza' } },
  { domain: domains.fr, prefix: 'fr', paths: { 'huis-ibiza': 'villa-ibiza' } },
  { domain: domains.it, prefix: 'it', paths: { 'huis-ibiza': 'villa-ibiza' } },
]

plugin.ts

Plugin.ts is a copy of the docs file.

@robisim74
Copy link
Owner

Ok, now the config if correct.

Uhm... the function is this: https://github.com/robisim74/qwik-speak/blob/main/packages/qwik-speak/tools/core/routing.ts

TypeError: Cannot read properties of undefined (reading 'map') would appear to be due to the fact that the rewriteRoutes you are passing are undefined

@robisim74
Copy link
Owner

import { domains } from './speak-config'; it seems a circular dependecies: move them in speak-routes.ts and check the position of import { rewriteRoutes } from "./src/i18n/speak-routes";

@fprl
Copy link
Author

fprl commented Dec 7, 2023

The circular dependency was the issue, thank you so much. Its amazing how you thought about all the routing cases.

@fprl
Copy link
Author

fprl commented Dec 7, 2023

Question, do you think its possible to have the same name for a route in different languages? I can see the issue here but I don't know if it can be solved due to how qwik handles routes.

built in 2.27s
[vite-plugin-qwik-city] Could not load /project/@qwik-city-plan (imported by src/s_fx0bdjeja0e.js): More than one route has been found for pathname "/villa-ibiza". Please narrow it down to only one of these:
  - /project/src/routes/huis-ibiza/index.tsx
  - /project/src/routes/huis-ibiza/index.tsx
  - /project/src/routes/huis-ibiza/index.tsx
  - /project/src/routes/huis-ibiza/index.tsx

@robisim74
Copy link
Owner

The problem is related to Qwik City and to the fact that the routing is file-based: therefore it finds multiple paths pointing to the same file.

Unfortunately it's a behavior I can't fix from this library.

@fprl
Copy link
Author

fprl commented Dec 7, 2023

Yes, I was sure it was that, Astro works the same way. Thank you again for pushing this :)

@fprl fprl closed this as completed Dec 7, 2023
@fprl
Copy link
Author

fprl commented Dec 7, 2023

One question about how extractFromDomain works. If I change my defaultLocale to english:
If extractFromDomain doesn't find a localized route, does it return lang from domain? i.e if route is not localized then default one is used? In this case villa-ibiza for all except dutch?

Because that way I could easily manage this, for example:

  • /huis-ibiza: dutch because extractFromDomain found the localized route in dutch.
  • /villas-ibiza (english domain): english because extractFromDomain didn't found localized route but matched domain
  • /villas-ibiza (german domain): german because extractFromDomain didn't found localized route but matched domain
  • ... rest

Does this makes sense?

@fprl
Copy link
Author

fprl commented Dec 7, 2023

export const extractFromDomain = (route: URL, domains: SpeakLocale[] | RewriteRouteOption[]): string | undefined => {
I see that it should return the domain that matches, will test it now!

EDIT: Works as expected, I should handle redirects manually in this case but its easy as only one path is localized.

@fprl
Copy link
Author

fprl commented Jun 10, 2024

The problem is related to Qwik City and to the fact that the routing is file-based: therefore it finds multiple paths pointing to the same file.

Unfortunately it's a behavior I can't fix from this library.

Hi Roberto, I'm wondering if the last version of qwik/qwik-city with this commit fix this issue? QwikDev/qwik#6375

Regards

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

No branches or pull requests

2 participants