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

Add getFragment method to ReactLocalization #595

Merged
merged 19 commits into from
Oct 27, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
194 changes: 194 additions & 0 deletions fluent-react/src/localization.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { FluentBundle, FluentVariable } from "@fluent/bundle";
import { mapBundleSync } from "@fluent/sequence";
import { Fragment, ReactElement, createElement, isValidElement, ReactFragment, cloneElement, ReactNode } from "react";
import { CachedSyncIterable } from "cached-iterable";
import { createParseMarkup, MarkupParser } from "./markup.js";
import voidElementTags from "../vendor/voidElementTags.js";

// Match the opening angle bracket (<) in HTML tags, and HTML entities like
// &amp;, &#0038;, &#x0026;.
const reMarkup = /<|&#?\w+;/;

/*
* `ReactLocalization` handles translation formatting and fallback.
Expand Down Expand Up @@ -73,6 +79,194 @@ export class ReactLocalization {
return fallback || id;
}

getFragment(
id: string,
args?: {
vars?: Record<string, FluentVariable>,
elems?: Record<string, ReactElement>,
},
fallback?: string
): ReactFragment {
return this.getElement(createElement(Fragment, null, fallback || id), id, args);
eemeli marked this conversation as resolved.
Show resolved Hide resolved
}

getElement(
component: ReactNode | Array<ReactNode>,
id: string,
args?: {
vars?: Record<string, FluentVariable>,
elems?: Record<string, ReactElement>,
attrs?: Record<string, boolean>;
},
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure about this. In <Localized/> passing each of these in as props makes sense, but here it feels like we're putting different things into the same basket -- esp. as this is so close to the shape of the getString() args argument.

My first instinct would be to separate vars from the other two, but maybe calling this something other than args could also help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh heh, good eye. I'd actually argue that renaming getStrings second argument to vars would be more appropriate, since the Fluent docs also call them variables, but I can understand that changing the name of an existing API might not be desirable.

I could name it something like props? Because they're basically analogous to <Localized>'s props, although it could also be confusing, given that they're not React props. Or how about params?

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with changing the internal name of the getSrting() argument to vars, because it only impacts the documentation.

props and params aren't really better, and both come with their own sets of baggage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eemeli marked this conversation as resolved.
Show resolved Hide resolved
): ReactElement {
let componentToRender: ReactNode | null;

// Validate that the child element isn't an array that contains multiple
// elements.
if (Array.isArray(component)) {
if (component.length > 1) {
throw new Error(
"Expected to receive a single React node element to inject a localized string into."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Expected to receive a single React node element to inject a localized string into."
"Expected to receive a single React element to localize"

Adding more words doesn't really help here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

);
}

// If it's an array with zero or one element, we can directly get the first
// one.
componentToRender = component[0];
} else {
componentToRender = component ?? null;
}

const bundle = this.getBundle(id);
if (bundle === null) {
if (!id) {
this.reportError(
new Error("No string id was provided when localizing a component.")
);
} else if (this.areBundlesEmpty()) {
this.reportError(
new Error(
"Attempting to get a localized element when no localization bundles are " +
"present."
)
);
} else {
this.reportError(
new Error(
`The id "${id}" did not match any messages in the localization ` +
"bundles."
)
);
}

return createElement(Fragment, null, componentToRender);
}

// this.getBundle makes the bundle.hasMessage check which ensures that
// bundle.getMessage returns an existing message.
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const msg = bundle.getMessage(id)!;

let errors: Array<Error> = [];

// Check if the component to render is a valid element -- if not, then
// it's either null or a simple fallback string. No need to localize the
// attributes.
if (!isValidElement(componentToRender)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would really rather prefer requiring componentToRender to be a valid element when using this method. For <Localized/> we do need to continue supporting use with no child, but would it be possible to check for that and handle it there, rather than needing to do so here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure what the best way to do that is - there'd be quite a bit of duplication in initialising the bundle and handling its various error cases. Personally I wouldn't be too worried about doing too much error handling. What I could do is update the type definition of getElement to only allow ReactChild | ReactFragment? Then we're technically requiring it to be a valid element in the public API.

Or, hmm, what I could also do is update <Localized> to check for non-element children and, if found, wrap them in a fragment and pass that to getElement? That would cause the latter to do some unnecessary work, but nothing too egregious I thinkg - what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

How about calling getString() rather than getElement() from a Localized with non-element children, and wrapping the result in a Fragment? As far as I can tell, that should replicate the current behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, that's clever: 59671cc

I should note that that does change the behaviour a little bit: if you do not provide a child, it will now render the string ID, rather than null. Which makes sense to me and aligns it with the existing behaviour of getString(), but I can also imagine that that's not desirable.

One way to work around that might be to set the fallback string to some unique string, and return null instead of a fragment if getString returns the fallback string, but that feels pretty hacky :/ Alternatively, although still a change in behaviour but closer to the previous behaviour, would be to set the fallback string to the empty string, resulting in it returning a fragment containing an empty string instead of null. That's technically different from what it used to be, but should have the same result for the user.

if (msg.value) {
// Replace the fallback string with the message value;
let value = bundle.formatPattern(msg.value, args?.vars, errors);
for (let error of errors) {
this.reportError(error);
}
return createElement(Fragment, null, value);
}

return createElement(Fragment, null, componentToRender);
}

let localizedProps: Record<string, string> | undefined;
// The default is to forbid all message attributes. If the attrs prop exists
// on the Localized instance, only set message attributes which have been
// explicitly allowed by the developer.
if (args?.attrs && msg.attributes) {
localizedProps = {};
errors = [];
for (const [name, allowed] of Object.entries(args?.attrs)) {
if (allowed && name in msg.attributes) {
localizedProps[name] = bundle.formatPattern(
msg.attributes[name],
args?.vars,
errors
);
}
}
for (let error of errors) {
this.reportError(error);
}
}

// If the component to render is a known void element, explicitly dismiss the
// message value and do not pass it to cloneElement in order to avoid the
// "void element tags must neither have `children` nor use
// `dangerouslySetInnerHTML`" error.
if (typeof componentToRender.type === "string" && componentToRender.type in voidElementTags) {
return cloneElement(componentToRender, localizedProps);
}

// If the message has a null value, we're only interested in its attributes.
// Do not pass the null value to cloneElement as it would nuke all children
// of the wrapped component.
if (msg.value === null) {
return cloneElement(componentToRender, localizedProps);
}

errors = [];
const messageValue = bundle.formatPattern(msg.value, args?.vars, errors);
for (let error of errors) {
this.reportError(error);
}

// If the message value doesn't contain any markup nor any HTML entities,
// insert it as the only child of the component to render.
if (!reMarkup.test(messageValue) || this.parseMarkup === null) {
return cloneElement(componentToRender, localizedProps, messageValue);
}

let elemsLower: Map<string, ReactElement>;
if (args?.elems) {
elemsLower = new Map();
for (let [name, elem] of Object.entries(args?.elems)) {
elemsLower.set(name.toLowerCase(), elem);
}
}

// If the message contains markup, parse it and try to match the children
// found in the translation with the args passed to this function.
const translationNodes = this.parseMarkup(messageValue);
const translatedChildren = translationNodes.map(({ nodeName, textContent }) => {
if (nodeName === "#text") {
return textContent;
}

const childName = nodeName.toLowerCase();

// If the child is not expected just take its textContent.
if (
!elemsLower ||
!Object.prototype.hasOwnProperty.call(elemsLower, childName)
) {
eemeli marked this conversation as resolved.
Show resolved Hide resolved
return textContent;
}

const sourceChild = elemsLower.get(childName);

// Ignore elems which are not valid React elements.
if (!isValidElement(sourceChild)) {
eemeli marked this conversation as resolved.
Show resolved Hide resolved
return textContent;
}

// If the element passed in the elems prop is a known void element,
// explicitly dismiss any textContent which might have accidentally been
// defined in the translation to prevent the "void element tags must not
// have children" error.
if (
typeof sourceChild.type === "string" &&
sourceChild.type in voidElementTags
) {
return sourceChild;
}

// TODO Protect contents of elements wrapped in <Localized>
// https://github.com/projectfluent/fluent.js/issues/184
// TODO Control localizable attributes on elements passed as props
// https://github.com/projectfluent/fluent.js/issues/185
return cloneElement(sourceChild, undefined, textContent);
});

return cloneElement(componentToRender, localizedProps, ...translatedChildren);
}

// XXX Control this via a prop passed to the LocalizationProvider.
// See https://github.com/projectfluent/fluent.js/issues/411.
reportError(error: Error): void {
Expand Down