Upgrade to fluent 0.8 and fluent-react 0.8. #3818

stasm · 2018-08-23T14:17:44Z

I've started working on upgrading the fluent dependencies to their most recent versions. Fixes #3789. Below is the summary of changes made in this PR.

`fluent 0.8.0`

Rename MessageContext to FluentBundle.

`fluent-react 0.6.0`

This version introduced React Overlays which allow translations to use some limited HTML markup.

Don't pass elements as $props to <Localized>. Instead, pass them as regular props. See details.
Add attrs to Localized if any attributes should be localized. See details. I didn't have to change anything because it looks like all impacted callsites already use attrs, which is great.
Don't use LocalizedHtml where a regular Localized would do. I was able to remove all but one uses of LocalizedHtml.

The one remaining use of LocalizedHtml is for experiments' introduction texts. They use arbitrary amount of markup which means that a single generic Localized cannot know which elements to pass as props.

Because all the other use-cases for using LocalizedHtml are gone, I was able to re-write it completely using the withLocalization HOC and tailor just to this one remaining use-case.

`fluent-react 0.7.0`

Use regular Localized also when HTML entities are present in the translation.

`fluent-react 0.8.0`

Rename the messages prop of LocalizationProvider to bundles.
Fix the build time error related to document being undefined.

To Do:

Rename affected message identifiers. In case of messages generated from yaml, do I use _l10nsuffix?
Test SSR. So far I ran npm run build:gulp-dist and it completed successfully. Is there more that I should test?
Use Fluent's DATETIME to format dates; remove the formatDate utility.

stasm · 2018-08-23T14:22:53Z

frontend/src/app/components/FeaturedExperiment/FeaturedButton.js

+      privacy-link={<a href="/privacy" onClick={(ev) => this.sendMetric(ev, {eventLabel: "Open general privacy"})}></a>}
+      modal-link={<a href="#" onClick={launchLegalModal}></a>}>
+        <p className="main-install__legal">
+            By proceeding, you agree to the <terms-link>terms</terms-link>{" "}


I'm not quite sure if using custom elements like that is a good practice in React. The contents of  will be replaced anyways by the translation, but before it happens, this fragment of the render tree will be constructed by React, I believe.

Perhaps it would be better to use a regular string?

 {`By proceeding, you agree to the <terms-link>terms</terms-link> and <privacy-link>privacy</privacy-link> policies of Test Pilot and <modal-link>this experiment's privacy policy</modal-link>.`} 

In the future, fluent-react might be able to extract en-US string from such elements. Right now, it would be just easier on the CPU.

stasm · 2018-08-23T14:24:56Z

frontend/src/app/components/RetireConfirmationDialog/index.js

@@ -37,9 +35,9 @@ export class RetireConfirmationDialog extends Component<RetireConfirmationDialog
              <Localized id="retireMessageUpdate">
                <p>As you wish. This will uninstall Test Pilot. You can disable individual experiments from the Firefox Add-ons Manager.</p>
              </Localized>
-              <LocalizedHtml id="retireEmailMessage">
+              <Localized id="retireEmailMessage" em={<em></em>}>


In future version of fluent-react, translations will be able to use text-level elements like  and   without requiring the code to pass them as props explicitly. See projectfluent/fluent.js#186. Once this lands, I'll file another PR to remove all these em={} et al.

stasm · 2018-08-23T14:26:52Z

frontend/src/app/containers/ExperimentPage/DetailsDescription.js

@@ -67,7 +67,7 @@ export default function DetailsDescription({
              </div>}
            <LocalizedHtml id={l10nId("introduction")}>
              <div>
-                {parser(introduction)}
+                {introduction}


IIUC, parsing the introduction text here uses up CPU cycles just for the localization to take over milliseconds later. This content will only be visible if the introduction id is missing from all locales, including English. I think this is unlikely enough to recognize this as a rare error scenario in which displaying unparsed HTML is acceptable.

stasm · 2018-08-23T14:57:43Z

The CI error is caused by UglifyJS not parsing the class syntax. According to my investigation from #3771 (comment), uglify-es used by uglifyjs-webpack-plugin should parse this file fine. So I guess there's something which runs some other minification step using uglify-js. Looking at package.json I suspect gulp-uglify. Can you help me track down where this minification happens?

stasm · 2018-08-23T15:01:07Z

Looking at package.json I suspect gulp-uglify. Can you help me track down where this minification happens?

Correction; I must have been looking at an old dependency graph. gulp-uglify is no more! (60bdfd2). Which leaves me puzzled about the CI error even more :)

meandavejustice · 2018-08-23T23:07:22Z

@stasm you should try pushing again now that @lmorchard has updated our webpack config to work with version 4

stasm · 2018-08-24T09:51:22Z

frontend/src/app/containers/ExperimentPage/DetailsDescription.js

@@ -85,11 +85,11 @@ export default function DetailsDescription({
                    </strong>
                  </Localized>}
                {copy &&
-                  <Localized id={l10nId(["details", idx, "copy"])}>
+                  <LocalizedHtml id={l10nId(["details", idx, "copy"])}>


None of the current experiments use markup for the details copy. It would be safer to use a regular <Localized> here.

stasm · 2018-08-24T09:51:39Z

frontend/src/app/containers/ExperimentPage/DetailsDescription.js

    >
-      <Localized id={l10nId("eolWarning")}>
+      <LocalizedHtml id={l10nId("eolWarning")}>


None of the current experiments use markup for the EOL warning. It would be safer to use a regular <Localized> here.

stasm · 2018-08-24T11:07:35Z

frontend/src/app/components/FeaturedExperiment/FeaturedButton.js

@@ -78,14 +77,17 @@ export default class FeaturedButton extends Component<FeaturedButtonProps, Featu
      this.sendMetric(ev, {eventLabel: "Popup Featured privacy"});
    };

-    return (<LocalizedHtml id={this.l10nId("legal-notice")}
-      $title={title}>
+    return (<Localized id={this.l10nId("legal_notice")}


I had to change the id here from legal-notice to legal_notice. Even if there's some normalization going on in the l10nId util, the code which looks up a possible _l10nsuffix uses the id verbatim, which resulted in searching for legal-notice_l10nsuffix. It might be a good idea to fix the experimentL10nId util in a future PR.

stasm · 2018-08-24T11:09:39Z

The CI error is related to Flow and my use of the FluentDateTime constructor from fluent/compat. I've fiddled with the flow-typed directory and with frontend/.flowconfig but without much success.

stasm

@flodolo The FTL files are ready for your review.

stasm · 2018-08-24T11:48:41Z

frontend/src/app/containers/ExperimentPage/DetailsOverview.js

    );
  }

-  const startedDate = formatDate(created);
+  const startedDate = new FluentDateTime(created, {


@flodolo Now that startedDate is a FluentDateTime object, do you think we should use the DATETIME builtin explicitly in the FTL? Like so:

-startedDateLabel = Experiment start date: {$startedDate} +startedDateLabel = Experiment start date: {DATETIME($startedDate)}

I don't even think we need to rev the id for this, but I'll leave the decision up to you.

If we do, we either need to rev the ID, or change the existing translation via script.

The current value of

startedDateLabel = Experiment start date: {$startedDate}

will continue to work, because Fluent recognizes the argument as a FluentDateTime anyways.

That's good, but nobody will realize they can actually customize the format if we don't add DATETIME().

That's a good point. Regardless of whether we need to rev the id, or use a script, would you rather use the version with DATETIME() or without?

I think it's good to have DATETIME(), it would be easy to help a locale customize that if needed.

flodolo

The number of changed strings grew significantly from the first time I checked the PR.

I wonder if we should try to write a script to udpate existing translations, when only the markup changes. It should be relatively straightforward to do for most of them, only one string, mobileDialogNoticeSMS, would require more work, and it would avoid losing a lot of translated content.

If we do that, we could even avoid using new string IDs.

Forgot one detail: translation via Pontoon happens in the #l10n branch. We need to make sure branches are in sync before this is merged, and to sync them back if we decide to change files via script.

flodolo · 2018-08-24T11:49:17Z

locales/en-US/app.ftl

@@ -36,7 +36,7 @@ headerLinkBlog = Blog
 landingIntroOne = Test new features.
 landingIntroTwo = Give your feedback.
 landingIntroThree = Help build Firefox.
-landingLegalNotice = By proceeding, you agree to the <a>Terms of Use</a> and <a>Privacy Notice</a> of Test Pilot.
+landingLegalNotice = By proceeding, you agree to the <terms-link>Terms of Use</terms-link> and <privacy-link>Privacy Notice</privacy-link> of Test Pilot.


This one still needs a new ID.

🤦‍♂️ I think I accidentally didn't commit this. Sorry, of course it needs a new id.

flodolo · 2018-08-24T11:49:50Z

locales/en-US/app.ftl

@@ -98,7 +98,7 @@ mobileDialogSuccessSecondary = Check your device for the email.
 mobileDialogAnotherDeviceLink = Send to another device
 mobileDialogError = Enter a valid email:
 mobileDialogErrorSMS = Enter a valid phone number or email:
-mobileDialogNoticeSMS = SMS service available in select countries only. SMS & data rates may apply. The intended recipient of the email or SMS must have consented. {$learnMore}
+mobileDialogNoticeSMS = SMS service available in select countries only. SMS & data rates may apply. The intended recipient of the email or SMS must have consented. <a>Learn more</a>.


Same here: we need a new ID, and not much we can do to avoid losing content.

flodolo · 2018-08-24T11:58:19Z

frontend/src/app/containers/ExperimentPage/DetailsOverview.js

    );
  }

-  const startedDate = formatDate(created);
+  const startedDate = new FluentDateTime(created, {


If we do, we either need to rev the ID, or change the existing translation via script.

stasm · 2018-08-24T12:34:39Z

The number of changed strings grew significantly from the first time I checked the PR.

Yeah, that's because the first few version of this PR didn't include changes to yaml files :(

stasm · 2018-08-24T12:58:08Z

@flodolo I updated the strings using DATETIME together with their ids. Over to you for deciding if we need a script to update the translations or not. Thanks!

flodolo

FTL looks good

flodolo · 2018-08-24T15:03:58Z

@stasm
Thoughts? I don't want to have a full parser and serializer for this…
https://gist.github.com/flodolo/d4f45a6da82f1cbb4d99a077ab7a8228

Test output. The l10n branch is behind, not having the Advance strings and the updated Lockbox one.

stasm · 2018-08-27T11:11:44Z

@flodolo The scrpit and the test output look good to me, thanks. In particular, I think it makes sense to consider mobileDialogNoticeSMSWithLink a new string.

stasm · 2018-08-27T11:25:42Z

Hey @lmorchard, can you help me fix the Flow errors in this PR please?

I'm going to be offline next week and the week following it. What's your advice wrt. to when to land this PR? Assuming I can get a green build today/tomorrow, would you be OK merging it around Wednesday? I'd like to be around for a couple of days after it lands, just in case there are any issues. If we don't merge this by Wednesday, I'd suggest waiting until I'm back from my PTO mid-September.

Since this PR touches strings, I'm also not sure if there's a schedule to follow wrt. the localization process. @flodolo, do you have a recommendation about when to best merge this PR?

flodolo · 2018-08-27T11:59:08Z

I don't have a strong preference, we just need to make sure l10n and master branches are in sync before merging, and sync back after merging this PR and running the script in a different PR. Doing it early this week is probably a good idea.

meandavejustice · 2018-08-27T18:32:16Z

@stasm Just tested this locally.

A solution for now would be to create a file at flow-typed/fluent.js with the following contents:

declare module 'fluent/compat' {
  declare module.exports: any;
}

This gets the flow tool passing. It would be nice to have flowtype definitions bundled along with fluent, maybe as a longer term goal.

codecov-io · 2018-08-28T06:43:25Z

Codecov Report

Merging #3818 into master will increase coverage by 0.5%.
The diff coverage is 68.96%.

@@            Coverage Diff            @@
##           master    #3818     +/-   ##
=========================================
+ Coverage   78.66%   79.17%   +0.5%     
=========================================
  Files         101      101             
  Lines        2827     2776     -51     
=========================================
- Hits         2224     2198     -26     
+ Misses        603      578     -25

Impacted Files	Coverage Δ
frontend/src/app/lib/utils.js	`80% <ø> (-2.15%)`	⬇️
...tend/src/app/components/MainInstallButton/index.js	`84.44% <ø> (-0.67%)`	⬇️
...pp/containers/ExperimentPage/IncompatibleAddons.js	`100% <ø> (ø)`	⬆️
...pp/containers/ExperimentPage/ExperimentControls.js	`72.72% <ø> (ø)`	⬆️
...d/src/app/components/ExperimentTourDialog/tests.js	`100% <ø> (ø)`	⬆️
frontend/src/app/components/Warning/index.js	`100% <ø> (+5.55%)`	⬆️
frontend/src/app/containers/RetirePage/index.js	`92.3% <ø> (ø)`	⬆️
...c/app/components/RetireConfirmationDialog/index.js	`89.47% <ø> (ø)`	⬆️
frontend/src/app/containers/ErrorPage.js	`100% <ø> (ø)`	⬆️
...rontend/src/app/components/NewsletterForm/index.js	`100% <ø> (ø)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16aa854...209583a. Read the comment docs.

stasm · 2018-08-28T07:12:42Z

@meandavejustice Thank you! I remember fiddling with the flow-types directory, but I didn't realize I could use any like you did.

The PR is now green. I've been testing it locally and things look good. In particular, I tested most of the strings which use markup, as well as the strings which display started and completed dates (which are now formatted by Fluent).

This PR is now ready for a review.

Also move fluent-react typings out of their own directory.

meandavejustice

Works great for me locally, not seeing any issues.

Thanks for taking care of this @stasm !

stasm · 2018-08-28T17:13:54Z

Thanks. I've pushed a small update to package.json to use fluent-react 0.8.1 which makes the peer dependency on fluent more explicit. Otherwise, the rest of the code is the same.

flodolo · 2018-08-28T17:18:33Z

@meandavejustice
We'll need to run this script on localization.

Ideally:

Sync l10n and master
Run the script
Sync l10n and master again

I can run the script and open a PR, but not sure I can help with the other two points.

stasm requested a review from flodolo as a code owner August 23, 2018 14:17

stasm commented Aug 23, 2018

View reviewed changes

meandavejustice requested a review from lmorchard August 23, 2018 15:34

stasm force-pushed the fluent-0.8 branch from b858092 to 76f687e Compare August 24, 2018 09:46

stasm commented Aug 24, 2018

View reviewed changes

stasm force-pushed the fluent-0.8 branch from 2ddbb91 to 377510e Compare August 24, 2018 11:04

stasm commented Aug 24, 2018

View reviewed changes

flodolo suggested changes Aug 24, 2018

View reviewed changes

stasm force-pushed the fluent-0.8 branch from 377510e to 7c91d91 Compare August 24, 2018 12:09

flodolo approved these changes Aug 24, 2018

View reviewed changes

meandavejustice mentioned this pull request Aug 24, 2018

fluent react error with email input wrapped in <Localized>in MobileDialog #3631

Closed

stasm force-pushed the fluent-0.8 branch from 7c2e71a to 94796b7 Compare August 28, 2018 06:55

stasm changed the title ~~WIP: Upgrade to fluent 0.8 and fluent-react 0.8.~~ Upgrade to fluent 0.8 and fluent-react 0.8. Aug 28, 2018

Upgrade to fluent 0.8, fluent-react 0.8

0fec93d

stasm added 5 commits August 28, 2018 18:34

Use fluent-react overlays where possible

dd1c97b

Rewrite LocalizedHtml to use withLocalization

66b5d4c

Use FluentDateTime in DetailsOverview

001e357

Update message ids of experiments' legal notices

fc69458

Add Flow typings for fluent/compat

209583a

Also move fluent-react typings out of their own directory.

meandavejustice self-requested a review August 28, 2018 16:58

meandavejustice approved these changes Aug 28, 2018

View reviewed changes

stasm force-pushed the fluent-0.8 branch from 94796b7 to 209583a Compare August 28, 2018 17:11

meandavejustice merged commit 2a5051b into mozilla:master Aug 28, 2018

stasm deleted the fluent-0.8 branch August 29, 2018 11:41

Upgrade to fluent 0.8 and fluent-react 0.8. #3818

Upgrade to fluent 0.8 and fluent-react 0.8. #3818

Conversation

stasm commented Aug 23, 2018 • edited Loading

To Do:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

stasm commented Aug 23, 2018

stasm commented Aug 23, 2018

meandavejustice commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

stasm commented Aug 24, 2018 • edited Loading

stasm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flodolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm commented Aug 24, 2018

stasm commented Aug 24, 2018

flodolo left a comment

Choose a reason for hiding this comment

flodolo commented Aug 24, 2018

stasm commented Aug 27, 2018

stasm commented Aug 27, 2018

flodolo commented Aug 27, 2018

meandavejustice commented Aug 27, 2018

codecov-io commented Aug 28, 2018 • edited Loading

Codecov Report

stasm commented Aug 28, 2018

meandavejustice left a comment

Choose a reason for hiding this comment

stasm commented Aug 28, 2018 • edited Loading

flodolo commented Aug 28, 2018

stasm commented Aug 23, 2018 •

edited

Loading

stasm Aug 23, 2018 •

edited

Loading

stasm Aug 24, 2018 •

edited

Loading

stasm commented Aug 24, 2018 •

edited

Loading

codecov-io commented Aug 28, 2018 •

edited

Loading

stasm commented Aug 28, 2018 •

edited

Loading