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

chore(payments): upgrade @fluent/bundle et al. #6098

Closed
wants to merge 4 commits into from

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Jul 31, 2020

Because

  • Fluent packages used to ship two build artifacts: index.js written in the most current ES version, and compat.js transpiled to ES2017. In Remove compat.js build artifacts projectfluent/fluent.js#472 we removed the compat.js builds and set tsc to target ES2018.
  • I didn't think it was appropriate for upstream Fluent to make the decision about the browsers supported by the compat.js builds. It's something that app developers should have more control over.
  • The expectation is that apps using Fluent will be able to transpile node_modules dependencies to match their browser compatibility requirements.

This pull request

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).

@stasm
Copy link
Contributor Author

stasm commented Jul 31, 2020

Other than transpilation, it's worth to mention that Fluent uses fairly new methods like Object.entries(), Array.prototype.includes(), and String.prototype.includes(). Looking at https://github.com/mozilla/fxa/blob/main/docs/supported-browsers.md I think all target browsers should support them. Do you run any cross-browser integration tests to ensure compatibility?

@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #6098 into main will increase coverage by 1.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6098      +/-   ##
==========================================
+ Coverage   93.84%   94.86%   +1.01%     
==========================================
  Files         299       59     -240     
  Lines       14417     1656   -12761     
  Branches      385      453      +68     
==========================================
- Hits        13530     1571   -11959     
+ Misses        884       84     -800     
+ Partials        3        1       -2     
Impacted Files Coverage Δ
packages/fxa-payments-server/src/App.tsx 100.00% <ø> (+33.33%) ⬆️
...payments-server/src/components/AppLayout/index.tsx 93.75% <ø> (ø)
...erver/src/components/PaymentConfirmation/index.tsx 100.00% <ø> (ø)
...yments-server/src/components/PaymentForm/index.tsx 100.00% <ø> (+1.35%) ⬆️
...ents-server/src/components/PaymentFormV2/index.tsx 100.00% <ø> (ø)
...-server/src/components/PaymentLegalBlurb/index.tsx 100.00% <ø> (ø)
...yments-server/src/components/PlanDetails/index.tsx 96.77% <ø> (+0.34%) ⬆️
...ts-server/src/components/TermsAndPrivacy/index.tsx 100.00% <ø> (ø)
...xa-payments-server/src/components/fields/index.tsx 99.04% <ø> (-0.03%) ⬇️
packages/fxa-payments-server/src/index.tsx 14.03% <ø> (ø)
... and 251 more

@vbudhram vbudhram requested a review from a team August 4, 2020 20:44
@chenba
Copy link
Contributor

chenba commented Aug 6, 2020

Let's rebase main once #6152 is merged to see if these conflicts will go away.

@chenba
Copy link
Contributor

chenba commented Aug 24, 2020

Closing in favor of #6291.

@chenba chenba closed this Aug 24, 2020
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