Shopify
fix issue where multiple ensureAuthenticated promises can be called
DO NOT MERGE
⏸️ PR IS ON HOLD ⏸️
WHY are these changes introduced?
We are still getting reports of users coming back to the CLI after leaving it running for a long period of time and seeing multiple authentication prompts / open browser tabs.
WHAT is this pull request doing?
Added a simple authentication lock to prevent concurrent authentication flows. When ensureAuthenticated is called, we check if authentication is already in progress. If one is in progress, we return the existing promise instead of starting a new one. All concurrent calls to ensureAuthenticated will share the same auth result.
How to test your changes?
- Pull down the branch
- Build the branch
- Run tests in
session.test.ts - Run
theme devand leave it open for as long as you're willing to see if there are any errors in re-auth (sessions refresh every 30 minutes). Also look to see if you are prompted multiple times or browser tab is opened multiple times. We should only see one if a session expires and needs manual prompting.
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
- [ ] n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
- [ ] Existing analytics will cater for this addition
- [ ] PR includes analytics changes to measure impact
Checklist
- [ ] I've considered possible cross-platform impacts (Mac, Linux, Windows)
- [ ] I've considered possible documentation changes
Coverage report
St.:grey_question: |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🟡 | Statements | 78.63% (-0.13% 🔻) |
13630/17334 |
| 🟡 | Branches | 72.68% (+0.2% 🔼) |
6624/9114 |
| 🟡 | Functions | 78.87% (+0.05% 🔼) |
3528/4473 |
| 🟡 | Lines | 78.97% (-0.13% 🔻) |
12880/16311 |
Show files with reduced coverage 🔻
St.:grey_question: |
File | Statements | Branches | Functions | Lines |
|---|---|---|---|---|---|
| 🟢 | ... / ConcurrentOutput.tsx |
98.36% (-1.64% 🔻) |
92% (-4% 🔻) |
100% | 98.33% (-1.67% 🔻) |
| 🔴 | ... / console.ts |
38.46% (-16.08% 🔻) |
100% | 0% | 38.46% (-16.08% 🔻) |
| 🔴 | ... / dev.ts |
17.39% (-7.61% 🔻) |
0% | 0% | 17.39% (-7.61% 🔻) |
| 🔴 | ... / init.ts |
28.57% (-1.43% 🔻) |
0% | 0% | 30% (-1.58% 🔻) |
| 🟡 | ... / language-server.ts |
66.67% (-13.33% 🔻) |
100% | 0% | 66.67% (-13.33% 🔻) |
| 🔴 | ... / open.ts |
50% (-33.33% 🔻) |
100% | 0% | 50% (-33.33% 🔻) |
| 🟡 | ... / package.ts |
66.67% (-13.33% 🔻) |
100% | 0% | 66.67% (-13.33% 🔻) |
| 🔴 | ... / profile.ts |
29.41% (-10.59% 🔻) |
0% | 0% | 29.41% (-10.59% 🔻) |
| 🔴 | ... / pull.ts |
44.44% (-18.06% 🔻) |
100% | 0% | 44.44% (-18.06% 🔻) |
| 🔴 | ... / push.ts |
50% (-16.67% 🔻) |
100% | 0% | 50% (-16.67% 🔻) |
| 🔴 | ... / share.ts |
50% (-5.56% 🔻) |
100% | 0% | 50% (-5.56% 🔻) |
| 🔴 | ... / pull.ts |
57.14% (-14.29% 🔻) |
100% | 0% | 57.14% (-14.29% 🔻) |
| 🟢 | ... / pull.ts |
96.77% | 69.7% (-0.57% 🔻) |
100% | 96.67% |
| 🟢 | ... / push.ts |
94.81% (-0.26% 🔻) |
80.65% (+3.01% 🔼) |
100% | 94.74% (-0.26% 🔻) |
| 🟢 | ... / theme-command.ts |
85.95% (-10.72% 🔻) |
73.13% (-15.27% 🔻) |
83.33% (-5.95% 🔻) |
87.27% (-10.89% 🔻) |
| 🔴 | ... / theme-ui.ts |
47.37% (-11.72% 🔻) |
36.36% (-2.53% 🔻) |
80% (+20% 🔼) |
47.37% (-11.72% 🔻) |
Test suite run success
3320 tests passing in 1380 suites.
Report generated by 🧪jest coverage report action from f1aa6052d31b3cd8f01f9bceb951e1139e7daaf1
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
🫰✨ Thanks @graygilmore! Your snapshot has been published to npm.
Test the snapshot by installing your package globally:
npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/[email protected]
[!CAUTION] After installing, validate the version by running just
shopifyin your terminal. If the versions don't match, you might have multiple global instances installed. Usewhich shopifyto find out which one you are running and uninstall it.
Thought this was already fixed 🤔
The refresh-token methods already have this functionality (only 1 refresh at a time),
why (or where) is theme dev calling ensureAuthenticated multiple times?
If you call ensureAuthenticated with no-prompt, then it won't ever open the browser on token refresh, is this still happening?
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. → If there's no activity within a week, then a bot will automatically close this. Thanks for helping to improve Shopify's dev tooling and experience.