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

feat(react): Reactify email-first/Index page #18498

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat(react): Reactify email-first/Index page #18498

wants to merge 1 commit into from

Conversation

LZoog
Copy link
Contributor

@LZoog LZoog commented Mar 3, 2025

Because:

  • We are converting our Backbone pages to React

This commit:

  • Adjusts content-server routing to make email-first servable by Express to React
  • Adds functionality from Backbone email-first to React email-first
  • Makes React email-first backwards compatible with Backbone until we feel confident in the full prod rollout, e.g. navigate vs hardNavigates
  • Adjusts stories/tests

fixes FXA-8636, fixes FXA-8072


TODO:

  • Non-React routes are not being served by Express properly, except for the Backbone index/email-first screen, e.g. going to /pair in this branch currently 404s. This is preventing me from testing RP redirect This is fixed.
  • Look at/do some stuff in the Backbone Express handler for React (e.g. MX stuff/redirects etc.)
  • Might need to look at InputText and prefillEmail again since I've got it as a placeholder but we want it to also be text in the input. Setting value makes it uneditable
  • Finish unit tests

Things that will still need doing here not yet captured in a ticket:

  • DNS email address lookup metrics
  • Probably a ticket to look at stricter oauth param validation or to at least log it in a ticket, since Backbone is more strict
  • Any needed functional test updates, but I think I may update our full prod rollout ticket to a 3-pointer to do these

Preferred way to test

  • Refresh back and forth between email-first and /clear until email-first enrolls you and takes you to /?showReactApp=true (which is what will happen for users).

You can also use ?forceExperiment=generalizedReactApp&forceExperimentGroup=react in a pinch but the new isInReactExperiment() in fxa-settings doesn't seem to always work with that. (This seems to add the value to local storage, so not sure off the top of my head why.)

Additionally you can go to /?showReactApp=true directly but similarly the isInReactExperiment() check won't work so navigating around back to / will load Backbone.

Things to test

  • location state updates instead of query params for Backbone, for example on reset password, clicking "Remember password? Signin" should prefill the email and not do a hard navigate
  • Backbone and React works
  • Sync signin/signup
  • RP signin, though we have a separate /oauth/ ticket for this. If you're using 123done and you click "Email-first", you can change the route from /oauth/ to / to test
  • Third party auth (note this currently appears to work, but I need to do more testing)
  • Metrics coming from React email-first and into flows work as expected
  • DNS lookup behaving as expected

export function useCheckReactEmailFirst() {
const config = useConfig();
// TODO with FXA-11221 (100% prod rollout), remove isInReactExperiment() check
return config.showReactApp.emailFirstRoutes === true && isInReactExperiment();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are the options I debated for this:

  • (what I went with) When we roll out React email first to 100%, which requires a 1-line code change, we make it a 2-line change instead and remove an isInReactExperiment() check
  • More work to access fullProdRollout from content-server react-app/index.js
  • Checking query param for prefillEmail / all other references, and make follow up to check location state later
  • Keep the hard navigate and hard reload the React bundle until we clean up later after stabilization
  • Take users to React email-first if feature flag is on, even if they aren't in the experiment

@LZoog LZoog force-pushed the FXA-8636 branch 4 times, most recently from c76efc8 to 8801e24 Compare March 5, 2025 22:28
@@ -0,0 +1,102 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, this was a port from email-domain-validator in content-server.


// Since we may perform an async call on initial render that can affect what is rendered,
// return a spinner on first render.
Copy link
Contributor Author

@LZoog LZoog Mar 6, 2025

Choose a reason for hiding this comment

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

I removed this because very few users (only those hitting signup directly with an email query param, which I think Sync/RPs may do in some cases) should have to wait for this call to complete, and if the account doesn't exist no rerender happens, but if the account does exist then those users will be redirected to /signin. You can test this by going to /[email protected] assuming you've got an account created with that email. It does mean users in this bucket will see signup flashed for a split second (however it's so fast that it's not even noticable locally).

The loading spinner on first render was less noticeable prior to React email-first but it makes the UI feel snappier to remove it now. I see looking now could do a useState for emailStatusChecked and return a loading spinner if not checked yet, but it could be overkill for a very specific edgecase. Depending on the amount of work left here maybe I'll circle back to it.

Because:
* We are converting our Backbone pages to React

This commit:
* Adjusts content-server routing to make email-first servable by Express to React
* Adds functionality from Backbone email-first to React email-first
* Makes React email-first backwards compatible with Backbone until we feel confident in the full prod rollout, e.g. navigate vs hardNavigates
* Adjusts stories/tests

fixes FXA-8636, fixes FXA-8072
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.

1 participant