i51: Deep Linking #79
Conversation
There was a problem hiding this comment.
Overall code looks amazing! Thanks so much for tackling this! One (hopefully) small thing.
What do you think about putting the button either by the url or as the url? I had a hard time finding it and once I did it felt out of place.
Just an idea so feel free to disagree! haha
|
Loved your design feedback @tscritch! Screen.Recording.2022-10-07.at.11.39.35.AM.mov |
There was a problem hiding this comment.
When I tested on a route that had no activity, copying/pasting the link worked, but on a route where there were 1+ responses I got 414 Request URI Too Large.
A few things that I noticed we were base64 encoding included markdown documentation for that route as well as request/response headers which could leak request headers (including auth headers).
If we focused on this just being URL/QS/Body Params and the route name/id, this would probably be lighter weight and safer. What are your thoughts here?
It could be also be interesting to explore adding ReactRouter (now or in the future) as a peer dependency and let the router do the heavy lifting for figuring out which route we're on. We'd still need to figure out body params, but that would likely knock out the path, url params, and QS params all with pre-existing route matching support.
|
Hey @jwdotjs thank you for testing the upper bound of this approach. I initially considered porting the app over to react-router, but I think we can achieve the desired behavior without such a big shift at the moment. GREAT call out with the leaked security headers - I have to admit I was lazy with my approach and I didn't notice all that metadata was included 😨. How do you feel about these latest changes where only the required data is encoded? If we would rather port over to react-router instead of encoding the URI I'm down with that as well! We could also look into hashing the URI or various string-shortening techniques if size is still a concern. |
|
Wow, thanks for the quick turnaround time. I was not expecting that! ❤️ I retested this morning and the URI size problem seems to be fixed and I think the type of data we're capturing all makes sense! I noticed that when I pasted the url, it seems to be not be loading the route definition correctly. If I get some free time today I'll try and see what might be causing that. 🙏 Route Definition With QS Params Specified
Pasted URL
|
|
Sorry for the delay on this. I was thinking of integrating React Router so we can have more documentation, but I've been blocking this for too long 😆 I'm going to look at the bug I posted in October, fix, and get this merged and tagged this weekend for us. 🙏 cc: @briansmithSR |
|
If the current approach is too complicated, could we just go with the original idea of only deep linking to the route? |
|
@justincy, yes, but this branch had bugs on it. I was working on it locally and I think it's in a better place, but I never had enough time to feel 100% confident. Let me push the updates to a branch targeting this branch for review |
|
Also note: I tried integrating React-Router and because it doesn't have control over the overall application and what route/path we're starting on, I was running into a lot of unexpected behaviors, so I more or less abandoned that idea in favor of getting this branch ready to merge. I think it's doable, but I just didn't have the time to work through the subtleties with components using a router when we don't know what path we started on |
|
I took a look at #90 and pulled out the primary fix for route definitions not loading (thanks for finding that, Jason!). I also fixed a bug where actual requests would break after deep linking depending on if the project provided I think everything is good to go now. Let me know if you would like to see a video of a specific thing - I know setting this project up to test can be a hassle haha. Screen.Recording.2023-09-04.at.2.11.11.PM.movScreen.Recording.2023-09-04.at.2.13.12.PM.movInvalid url just goes home Screen.Recording.2023-09-04.at.2.24.57.PM.mov |
This PR adds support for copying a deep link to a request.
Closes #51
Steps to test
Video Demo
Screen.Recording.2022-10-07.at.11.39.35.AM.mov