-
-
Notifications
You must be signed in to change notification settings - Fork 237
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: AOT compilation with icu-to-json
(experiment)
#705
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -1,9 +1,7 @@ | |||
import {render, screen} from '@testing-library/react'; | |||
import {parseISO} from 'date-fns'; | |||
// eslint-disable-next-line import/no-named-as-default -- False positive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -412,7 +412,8 @@ describe('t.rich', () => { | |||
); | |||
}); | |||
|
|||
it('handles nested rich text', () => { | |||
// TODO: icu-to-json doesn't seem to handle this currently | |||
it.skip('handles nested rich text', () => { | |||
const {container} = renderRichTextMessage( | |||
'This is <bold><italic>very</italic> important</bold>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be addressed on the icu-to-json
side I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this "should" work 😄
you might have to define a tag
and/or a baseTag
formatter
| string | ||
| { | ||
type: number; // TODO: Unused, is this necessary? | ||
tokens: Array<unknown>; // TODO: Unused, is this necessary? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these two properties be removed from the parsed result? Or should they be used by the consumer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean - Formatters are called here with (value, locale, formatOptions)
the format options are passed through from MessageFormat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean e.g. these cases:
// [["price",4,"numberFmt",{"type":0,"tokens":[{"stem":"currency","options":["EUR"]}],"parsedOptions":{"style":"currency","currency":"EUR"}}]]
'{price, number, ::currency/EUR}'
// [["value",4,"numberFmt",{"type":0,"tokens":[{"stem":".#","options":[]}],"parsedOptions":{"maximumFractionDigits":1}}]]
'{value, number, ::.#}'
Is tokens
necessary here? In regard to minimizing the size of the AST, could this be simplified to something like this?
[["value",4,"numberFmt",{"maximumFractionDigits":1}]]
Similarly, date skeletons seem to be a bit verbose:
// [["now",4,"date",{"type":1,"pattern":"yyyyMdHm","parsedOptions":{"year":"numeric","month":"numeric","day":"numeric","hourCycle":"h23","hour":"numeric","minute":"numeric"}}]]
'{now, date, ::yyyyMdHm}'
Could the parsedOptions
be unwrapped and replace the object they're placed within?
type MessageFormat = Omit< | ||
ReturnType<typeof compile>, | ||
// TODO: Do we need the args? | ||
'args' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be removed from the parsed result? See also https://github.com/amannn/next-intl/pull/705/files#r1418720864
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use compileToJson
instead it's the same as compile
just without args
args
are helpful to generate types or to optimize which formatters should be included to a bundle.
e.g. if date is used you might want to add a date formatter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks!
icu-to-json
icu-to-json
(experiment)
const evaluated = evaluateAst( | ||
messageFormat.json, | ||
locale, | ||
allValues, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I pass the values from the user or args
from the parsed result here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the args are not needed for formatters however the args can tell you during compile time which formatters need to be available to run the code.
in your case there is a very special formatter wich you should add for full react support
it allows processing the children before they are passed to the given tag function:
{
tag: (children: Array<string | ReactNode>, locale: string) =>
children.map((value, i) => typeof value === "string"
? value
: <Fragment key={`f-${i}`}>{value}</Fragment>
}
e.g. for a translation like <foo>Hello <b>{name}</b></foo>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also the baseTag
formatter which is a fallback which kicks in if the end-user did not define a tag.
by default the tag is converted to a string but you might also wrap in a fragment or even allow some tags to be used as html:
// for a compiled message "<b>Hello {name}</b>"
const formatters = {
baseTag: (Tag, chilren) => {
// allowlist:
if (["b", "strong", "p"].include(Tag)) {
return <Tag>{children}</Tag>
}
return <>{children}</>;
}
};
run(message, lang, { name: "Joe" }, formatters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried updating your example here to use nested rich text: https://codesandbox.io/p/sandbox/icu-to-json-demo-forked-2jzngr?file=%2Fsrc%2Ficu.tsx%3A19%2C1-20%2C1
Somehow the inner part is missing. Am I doing something wrong?
// to be able to format to parts. | ||
prepareTranslationValues({...defaultTranslationValues, ...values}) | ||
const allValues = {...defaultTranslationValues, ...values}; | ||
// TODO: The return type seems to be a bit off, not sure if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are two different ways to run the precompiled icu messages:
run
evaluateAst
the idea is that run
will always return a string and evaluateAst
will always return an array.
const t = (key, args) => run(messages[key], lang, args);
icu-to-json
is framework independent so it would require you to wrap a react Fragment around the result:
t.rich = (key, args) => <>{evaluateAst(messages[key], lang, args)}</>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that helps with the fragment—thanks!
I was wondering about this case:
It seems like the args
determine the return value. However, the function b
has already been called at this point (having returned a React.JSX.Element
) and date
should be turned into a string.
Therefore the returned value should have the type (string | React.JSX.Element)[]
, right?
Or am I missing something?
icu-to-json
(experiment)icu-to-json
(experiment)
Fixes #962
(WIP)
This is an experiment to use
icu-to-json
for AOT compilation of messages on the server side / at build time and to reduce the bundle size in Client Components.Current progress: For now, both compilation as well as evaluation happens in the same module (and therefore on the client side). This is mostly intended to evaluate initial compatibility. Later, we could move the compilation step to the server.
Questions:
t.raw
ort.markup
? We need to decide if a message needs to be parsed before it's used. Do we need to analyze the usages? Or could we revert the parsing whent.raw
is called?TODO