-
Notifications
You must be signed in to change notification settings - Fork 217
feat(sync): Implement browser service=relay logic and UX #17869
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
Conversation
packages/fxa-dev-launcher/README.md
Outdated
| - `FXA_DESKTOP_CONTEXT` - context value for the fxa-content-server: `context=[value]` (NOTE: `fx_desktop_v3` is default). | ||
|
|
||
| Tip: run `FXA_DESKTOP_CONTEXT=oauth_webchannel_v1 yarn firefox` to test the OAuth Desktop flow. | ||
| Tip: run `FXA_DESKTOP_CONTEXT=oauth_webchannel_v1 yarn firefox` to test the OAuth Desktop flow. Run `FXA_DESKTOP_CONTEXT=oauth_webchannel_v1 FIREFOX_BIN=/Applications/Firefox\ Nightly.app/Contents/MacOS/firefox yarn firefox` to test the OAuth Desktop flow (for MacOS) on Nightly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If run from the fxa-dev-launcher, the command would instead be:
FXA_DESKTOP_CONTEXT=oauth_webchannel_v1 FIREFOX_BIN=/Applications/Firefox\ Nightly.app/Contents/MacOS/firefox yarn start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, to run on local, will need to add FXA_ENV=local. Without it, the dev launcher was defaulting to prod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I just put a note to run it from the root repo? I hardly ever run it directly in the package 😅 Using that command too, it seems to default to FXA_ENV=local.
edit: added a note to run that command from root
6622aa4 to
71de5e0
Compare
Because: * We want to support this new 'sync optional' flow to allow users to sign into the browser without being signed into Sync, beginning with Relay This commit: * Allows service=relay query param with our oauth flow * Adds logic to content-server reliers/fxa-settings integrations to check for this browser-but-not-Sync Relay case * Makes conditional email-first UX changes for service=relay * Makes conditional Signup UX changes for service=relay * Adjusts web channel messages for fxaLogin and fxaOAuthLogin, for signin and signup closes FXA-10313, closes FXA-10377
| <Subject /> | ||
| </AppLayout> | ||
| ); | ||
| export const FancySuccess = () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason this is included here is because I thought I was going to be implementing the UX in Figma for confirm signup code and wanted to see what this looked like, but that'll be done in a separate ticket.
| - `FXA_DESKTOP_CONTEXT` - `context=` value. (NOTE: `fx_desktop_v2` is default). | ||
| - `FIREFOX_BIN=/Applications/FirefoxNightly.app/Contents/MacOS/firefox-bin npm start` | ||
| - `FIREFOX_DEBUGGER=true` - open the [Browser Toolbox](https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox) on start (NOTE: `false` by default for speed). | ||
| - `FXA_DESKTOP_CONTEXT` - context value for the fxa-content-server: `context=[value]` (NOTE: `fx_desktop_v3` is default). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to your patch directly but the property you are documenting: there is one other FXA_DESKTOP_CONTENT doc line for this property (line 10) which is incorrect and duplicated by this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will get this modified in another quick PR to avoid CI rerun here, thanks!
vbudhram
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LZoog Code wise the LGTM, gonna do some local testing before r+
| services: { | ||
| sync: {}, | ||
| }, | ||
| // OAuth desktop sync optional flow sends service in fxaOAuthLogin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for updating comments 👍🏽
|
👍 worked great when I tested manually. |
vbudhram
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LZoog Works well locally, thanks!
Because:
This commit:
closes FXA-10313, closes FXA-10377
This needs to be tested with latest Nightly (and then swapping
service=syncwithservice=relay), see fxa-dev-launcher README with new (in this branch) Nightly command note for oauth desktop.