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

feat(config): add site.url as an alternative to baseUrl #2481

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dargmuesli
Copy link
Collaborator

πŸ”— Linked issue

#2474

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR adds support for reading Nuxt config's site.url, provided by Nuxt SEO's site-config module, marking i18n.baseUrl as deprecated.

Draft because:

  • nuxt-site-config does not (yet?) support options as functions, which has to be discussed

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@nuxt-studio
Copy link

nuxt-studio bot commented Oct 9, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
i18n Edit on Studio β†—οΈŽ View Live Preview 7931245

@@ -90,6 +90,8 @@
"knitwork": "^1.0.0",
"magic-string": "^0.30.4",
"mlly": "^1.4.2",
"nuxt-site-config": "^1.4.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harlan-zw https://nuxtseo.com/site-config/getting-started/installation specifies that nuxt-site-config has to be installed along with nuxt-site-config-kit. As I only ever made use of a nuxt-site-config-kit import in this PR and ignoring the playground's useSiteConfig usage right now, I wonder: is it really required to install nuxt-site-config as well? And if so, why?

useScope: 'local'
})
console.log('locales', locales, baseUrl)
const { url } = useSiteConfig()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: this should require enabling the nuxt-site-config module on the playground.

specs/seo/baseUrl.spec.ts Outdated Show resolved Hide resolved
@BobbieGoede
Copy link
Collaborator

While I'm in favor of supporting/using the config module, if we want to add this for v8 it will have to be backwards compatible. I feel it's important that we make an effort to prevent breaking changes between release candidates as we are working to stable release.

So hopefully there is a way to implement this while keeping compatibility and otherwise we will have to delay this until after v8 stable release.

@dargmuesli
Copy link
Collaborator Author

This change should be backwards compatible already and only make use of site.url if available. The only thing a user should see differ is the deprecation notice, which could theoretically be removed and reintroduced before v9.
I haven't done extensive testing yet though as I first want to resolve the threads above I opened myself before doing testing.

src/module.ts Outdated
@@ -72,6 +73,12 @@ export default defineNuxtModule<NuxtI18nOptions>({
)
}

if (options.baseUrl && options.baseUrl !== '') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a JSDoc deprecation should be enough without this. It's not really a problem if people want to keep their i18n config together

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would I add the JSDoc as that's picked from vue-i18n-routing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would I add the JSDoc as that's picked from vue-i18n-routing?

Something like this is possible, I think the original JSDoc would need to be copied and extended, unless there is some way to append to existing JSDoc that I'm unaware of πŸ˜….

+  /**
+   * Custom JSDoc here!
+   */
+  baseUrl?: I18nRoutingOptions<Context>['baseUrl']
} & Pick<
  I18nRoutingOptions<Context>,
- | 'baseUrl'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added πŸš€

src/module.ts Outdated
const siteConfig = useSiteConfig()

if (siteConfig.url) {
options.baseUrl = siteConfig.url

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? We want to use set the baseUrl in a runtime environment so that it can be easily switched

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah fair. Yeah, currently I only set it right here at build time, that should change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ea4a900 I made that change. There could be a better place of course as @BobbieGoede pointed out, but I don't feel knowledgeable enough to decide on that.

Use `site.url` instead of `i18n.baseUrl`.

::alert{type="info"}
The `site` configuration option stems from [Nuxt SEO's `site-config` module](https://nuxtseo.com/site-config), which `nuxt-i18n` makes use of now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there will need to be some documentation on all the ways that site config can be set and hosted on this page. The docs aren't that friendly at the moment, something on my to-do list.

@@ -83,6 +83,38 @@ Deprecated for the same reason as the `localeLocation()` Option API.

Use `localeHead()` instead for the Options API style.

### Deprecated `baseUrl` component option

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually need to deprecate it, both would be valid and we can just push people to use site config instead. If they really want to use baseUrl then they should be free to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the deprecations.

@harlan-zw
Copy link

harlan-zw commented Oct 10, 2023

@BobbieGoede would you mind sharing what your understanding of the baseUrl does? I want to make sure we're covering the test cases and actually implementing the module makes sense.

From my understanding, it's just used for i18n head tags?

A couple of minor notes until I can do a proper review:

  • end user shouldn't have to migrate baseUrl to site.url, both should be valid - for now (we nudge towards site url for easier consolidation of config)
  • worth consolidating the trailingSlash config which nuxt-site-config also supports

@BobbieGoede
Copy link
Collaborator

@dargmuesli

This change should be backwards compatible already and only make use of site.url if available. The only thing a user should see differ is the deprecation notice, which could theoretically be removed and reintroduced before v9.

Ah right, my bad. I mistook the deprecation notices as having the original baseURL handling removed for some reason πŸ˜….

It's good to support setting the value using nuxt-site-config, but deprecating baseUrl will be something to decide later/separately. For simplicity (for users) I think it is important to keep the ability to configure the module in a single place (the i18n key or module array).

I haven't done extensive testing yet though as I first want to resolve the threads above I opened myself before doing testing.

One thing to consider is layer support, especially when supporting multiple ways of setting options. In the current implementation a layer will override a project's baseUrl if the layer uses this new approach.

@harlan-zw

From my understanding, it's just used for i18n head tags?

As I understand it, this is correct. Most of this happens in vue-i18n-routing, which uses the config to set up the head tags*.

It is possible to override baseUrl by setting domain on locales, or by using a function to set it based on the Nuxt Context as seen here. When setting domain on locales, it is possible to override these using runtime config as well. These options exist to allow projects to support different languages using different domains, I'm not sure how this would interact with nuxt-site-config or when other modules rely on it as well.

*On a separate note, I've been wanting to look into implementing/replacing the head tag handling in vue-i18n-routing with unhead. Unfortunately right now I'm not yet familiar enough with either of these packages to do this, if you have time to give it a look and/or point me in the right direction, that would be greatly appreciated! πŸ˜…

@dargmuesli dargmuesli self-assigned this Oct 19, 2023
@kazupon
Copy link
Collaborator

kazupon commented Nov 13, 2023

Sorry, I was preparing for vue fes japan, so I could not track of what was going on with this issue. πŸ˜…
How is going this PR?

@dargmuesli
Copy link
Collaborator Author

It's on my TODO list, but as it should be a backwards compatible feature addition I had other tasks with higher priority. I think I can continue to work on it this week.

@harlan-zw
Copy link

I also need to re-review this properly when I have some time

Copy link
Collaborator Author

@dargmuesli dargmuesli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the deprecations, added separate site.url tests besides the baseUrl tests and moved the site config to the runtime. That should form the groundwork I intended to lay. For further changes I'd need assistance from you people ❀️

Be invited to give examples on what needs to change or to submit PRs against this PR's base branch in my fork to get site config integrated into nuxt-i18n! πŸ’ͺ πŸ₯³

@dargmuesli dargmuesli changed the title feat(config): deprecate baseUrl in favor of site.url feat(config): add site.url as an alternative to baseUrl Nov 16, 2023
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.

4 participants