-
Notifications
You must be signed in to change notification settings - Fork 10
VIDSOL-134: add route to assetlinks json file #202
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
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.
Looks good! 💪
That said, left some questions/comments. Thanks! 🙏
backend/.well-known/assetlinks.json
Outdated
| @@ -0,0 +1,9 @@ | |||
| [{ | |||
| "relation": ["delegate_permission/common.handle_all_urls"], | |||
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.
It's not obvious what the relation properties listed provide us. Since this is a JSON file, we don't have a good way of inlining documentation either.
Is there a way for us to document why the non-obvious properties have the value they do? Naively, maybe we can document them in the integration test you added?
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.
Thinking about this some more, you can probably make this a plain object or refactor as a getter that returns a plain object. Then, you can add comments, as needed.
The endpoint you created below can handle the serialization and return the expected JSON.
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.
changes done here 11d3528
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.
LGTM! 💪 🚀
| assetLinksRouter.get('/assetlinks.json', (_req: Request, res: Response) => { | ||
| res.json([ | ||
| { | ||
| // Grants the target permission to handle all URLs that the source can handle |
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.
Nice comments! 💪
backend/routes/assetlinks.ts
Outdated
| const assetLinksRouter = Router(); | ||
|
|
||
| /** | ||
| * tl:dr: Serve the static file needed for mobile deep linking |
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.
tl:dr: implies that this will be preceded or proceeded by a more verbose explanation. Since there is none, how do you feel about dropping it (and leaving "Serve the ...")?
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.
changed here 568248a
4e43d6b to
42e7e59
Compare
42e7e59 to
5b92bcd
Compare
|
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.
LGTM Great job! ![]()
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.
LGTM! 🚀



What is this PR doing?
Serve the static file
assetlinks.jsonin the URL{baseUrl}/.well-known/assetlinks.jsonfollowing the documentation https://developer.android.com/training/app-links/verify-android-applinks#publish-jsonHow should this be manually tested?
N/A -- make sure CI passes (since an integration test was added)
What are the relevant tickets?
Resolves VIDSOL-134
Checklist
develop(notmain).Known Issue.docs/KNOWN_ISSUES.md?Issues.If yes, which issue? Issue Number?