Skip to content

Add FluentResource #244

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

Merged
merged 3 commits into from
Jul 10, 2018
Merged

Add FluentResource #244

merged 3 commits into from
Jul 10, 2018

Conversation

zbraniecki
Copy link
Collaborator

This implements the first part of https://bugzilla.mozilla.org/show_bug.cgi?id=1455649 in fluent.js allowing for caching of parsed resources.

@zbraniecki zbraniecki requested a review from stasm July 6, 2018 20:35
Copy link
Contributor

@stasm stasm left a comment

Choose a reason for hiding this comment

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

This looks good code-wise, but before approving the PR I'd like to learn your opinion on where the parsing logic should live. See #217 (comment) for @Pike's thoughts on this. I think I agree: it would likely make more sense for the parser to be part of FluentResource. Given that this is supposed to help the Gecko use-case, could you weigh on which approach is the best from the Gecko POV?

@@ -115,22 +131,45 @@ export class MessageContext {
* @returns {Array<Error>}
*/
addMessages(source) {
const [entries, errors] = parse(source);
for (const id in entries) {
const res = MessageContext.parseResource(source);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use this here rather than MessageContext to allow for subclassing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using this throws on babel unfortunately here :(

@zbraniecki
Copy link
Collaborator Author

Given that this is supposed to help the Gecko use-case, could you weigh on which approach is the best from the Gecko POV?

I'm not strongly opinionated here. Happy to go either way.

Eventually there may be some complication when we try to transfer FluentResource between processes. I'm not sure how much hassle it adds to have methods defined on the data structure there, but we can cross that bridge when we come to it.

@zbraniecki
Copy link
Collaborator Author

Should I move the parser to FluentResource then, @stasm?

@stasm
Copy link
Contributor

stasm commented Jul 10, 2018

Eventually there may be some complication when we try to transfer FluentResource between processes. I'm not sure how much hassle it adds to have methods defined on the data structure there, but we can cross that bridge when we come to it.

My hope and the intent behind the extends Map design is that we will be able to "downgrade" to a regular Map in IPC scenarios, which the structured clone algorithm supports. This would happen automatically whenever a FluentResource instance is postMessaged.

Once its FTL is parsed, FluentResource doesn't really need any parsing capabilities. MessageContext.addResource can effectively take anything with an interface of a Map.

So yeah, let's move parsing to FluentResource, please. It's OK to keep it the code in parser.js and import it into resource.js. We don't need to expose parsing as a public method of FluentResource AFAICT.

@zbraniecki zbraniecki requested a review from stasm July 10, 2018 15:33
Copy link
Contributor

@stasm stasm left a comment

Choose a reason for hiding this comment

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

Sweet, thanks!

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