[cli] try to detect if deep link was successful #18839
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ENG-6120 Take user to interstitial when launching dev client project from versioned CLI
|
esamelson
force-pushed
the
@eric/eng-6120-3
branch
from
d47fcd8
to
0c895d9
Compare
lukmccall
approved these changes
Aug 26, 2022
EvanBacon
reviewed
Aug 29, 2022
| @@ -70,7 +70,9 @@ export class MetroBundlerDevServer extends BundlerDevServer { | |||
| // https://github.com/expo/expo/issues/13114 | |||
| prependMiddleware(middleware, manifestMiddleware); | |||
|
|
|||
| middleware.use(new InterstitialPageMiddleware(this.projectRoot).getHandler()); | |||
| middleware.use( | |||
| new InterstitialPageMiddleware(this.projectRoot, options.location.scheme).getHandler() | |||
There was a problem hiding this comment.
The other middleware uses the format of projectRoot, { options as object }
There was a problem hiding this comment.
The scheme here is capable of being dynamic, seems like we shouldn't pass in the default value.
There was a problem hiding this comment.
Could you point me to where the scheme should come from then? Do you mean retrieve it using getOptionalDevClientSchemeAsync inside of the InterstitialPageMiddleware (and fall back to this value if that returns null)?
There was a problem hiding this comment.
I recommended a TODO, for now. I can implement this later since it's an edge case they may only happen theoretically.
EvanBacon
reviewed
Aug 29, 2022
packages/@expo/cli/src/start/server/middleware/InterstitialPageMiddleware.ts
Outdated
Show resolved
Hide resolved
EvanBacon
reviewed
Aug 29, 2022
packages/@expo/cli/src/start/server/middleware/InterstitialPageMiddleware.ts
Outdated
Show resolved
Hide resolved
EvanBacon
reviewed
Aug 29, 2022
packages/@expo/cli/src/start/server/middleware/InterstitialPageMiddleware.ts
Outdated
Show resolved
Hide resolved
EvanBacon
reviewed
Aug 29, 2022
packages/@expo/cli/src/start/server/middleware/__tests__/InterstitialPageMiddleware-test.ts
Outdated
Show resolved
Hide resolved
EvanBacon
reviewed
Sep 8, 2022
esamelson
force-pushed
the
@eric/eng-6120-2
branch
from
6112afd
to
e7c6a4b
Compare
esamelson
force-pushed
the
@eric/eng-6120-3
branch
2 times, most recently
from
0b56b1c
to
ed6174f
Compare
…r.ts Co-authored-by: Evan Bacon <bacon@expo.io>
esamelson
force-pushed
the
@eric/eng-6120-3
branch
from
ed6174f
to
0337efb
Compare
EvanBacon
requested review from
Simek,
amandeepmittal and
kbrandwijk
as code owners
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
fourth step of ENG-6120; ports expo/expo-cli@9777ad6 and the relevant portions of expo/expo-cli@b5c59a7 to the new CLI
How
Test Plan
Added unit tests for all new functionality and updated existing tests. Tested manually to ensure the page looks as expected (parity with old CLI).
Checklist
expo prebuild& EAS Build (eg: updated a module plugin).