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: change createMessagesDeclaration to also accept array for monorepo support #1700

Merged
merged 5 commits into from
Feb 5, 2025

Conversation

felix-quotez
Copy link

@felix-quotez felix-quotez commented Feb 5, 2025

Resolves: #1699

This change renames "createMessagesDeclaration" to "createMessagesDeclarations" and changes the type to an array. Each entry of the array will be used to create message declaration files. Supporting multiple message files helps to use next-intl in monorepo setups.

Since v4 is in beta, I renamed the property and removed support for a single string. To me, clean types lead to cleaner code. If compatibility is a more of a concern during the beta phase, I could not change the property name and keep the support for single strings. Please let me know.

Copy link

vercel bot commented Feb 5, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-intl-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 5, 2025 4:48pm
next-intl-example-app-router ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 5, 2025 4:48pm
next-intl-example-app-router-without-i18n-routing ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 5, 2025 4:48pm

Copy link

vercel bot commented Feb 5, 2025

@felix-quotez is attempting to deploy a commit to the next-intl Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

Hey, thanks for proposing this PR!

I've left a note about keeping the previous API to keep the common case simpler, but I think it should be fine to extend this.

As a side note, can you share how you're importing messages from another repo? I know that other users have struggled with this: #1688. Might be worth verifying this works with the proposed changes.

@@ -1,6 +1,6 @@
export type PluginConfig = {
requestConfig?: string;
experimental?: {
createMessagesDeclaration?: string;
createMessagesDeclarations?: Array<string>;
Copy link
Owner

Choose a reason for hiding this comment

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

Can we keep the existing signature to make the common case easy?

Suggested change
createMessagesDeclarations?: Array<string>;
/** A path to the messages file that you'd like to create a declaration for. In case you want to consider multiple files, you can pass an array of paths. */
createMessagesDeclaration?: string | Array<string>;

@felix-quotez
Copy link
Author

Pushed a fix, the type is now "string | Array;" and the property continues to be called the singular "createMessagesDeclaration". Please have another look.

As a side note, can you share how you're importing messages from another repo? I know that other users have struggled with this: #1688. Might be worth verifying this works with the proposed changes.

I have a special setup so I am not sure how helpful this is for others. It's for sure different from the linked discussion/request in that I enjoy that each app/package has their own messages. In my setup I am combining the @formatjs/cli for extracting messages with next-intl (the goal is to get the "colocation" benefit outlined here https://formatjs.github.io/docs/getting-started/message-declaration/). Using the CLI also nicely gives me an en.json file per package. In the request.ts file I need to manually import all the relevant messages file--it not ideal but also not too concerning to me.

@felix-quotez felix-quotez changed the title feat: change createMessagesDeclaration to an array for monorepo support feat: change createMessagesDeclaration to also accept array for monorepo support Feb 5, 2025
Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot!

I enjoy that each app/package has their own messages

I'd do that to. Sorry, I just noticed I've linked to the wrong discussion, this one seems more related: #1224.

In my setup I am combining the @formatjs/cli for extracting messages with next-intl (the goal is to get the "colocation" benefit outlined here https://formatjs.github.io/docs/getting-started/message-declaration/).

What, really? 😄 Can you give an example how the usage of translations in a component looks like? Do you use defineMessages along with useTranslations from next-intl in a component?

packages/next-intl/src/plugin/types.tsx Show resolved Hide resolved
@amannn
Copy link
Owner

amannn commented Feb 5, 2025

Out in [email protected]!

@felix-quotez
Copy link
Author

felix-quotez commented Feb 5, 2025

Thanks for merging the PR! 🥳

Do you use defineMessages along with useTranslations from next-intl in a component?

Yes, this is exactly what I do. (I simply re-exported these function from our internal package so that it looks good in the components, both, defineMessages and useTranslations are unchanged.) I am using a custom formatter, that takes the formatjs output, and formats in the way next-intl needs it. So it gives full type safety--and now also with type safe arguments thanks to your work!

import { defineMessages } from '@tw/intl/define-messages'
import { useTranslations } from '@tw/intl/use-translations'

defineMessages({
  propertyNameIsIgnoredInFavorOfIdBelow: {
    id: 'a.b',
    description: 'foo',
    defaultMessage: 'bar',
  },
})

export function Component() {
  const t = useTranslations('a')
  return t('b')
}

Edit: The worst part about this is that the files need to be manually watch to trigger extracting message with formatjs. I don't think they support a "--watch" option.

@amannn
Copy link
Owner

amannn commented Feb 5, 2025

That's really interesting, thanks for sharing!

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