Redirects for dev mode short links #465
Conversation
aomarks
requested review from
sorvell,
justinfagnani,
arthurevans and
kevinpschaaf
|
A live preview of this PR will be available at the URL(s) below. https://pr465-57420cb---lit-dev-5ftespv5na-uc.a.run.app/ |
There was a problem hiding this comment.
This looks really great and a huge ergonomic improvement using Lit. I tried out manually some short urls:
These work great:
- https://pr465-57420cb---lit-dev-5ftespv5na-uc.a.run.app/msg/removed-api
- https://pr465-57420cb---lit-dev-5ftespv5na-uc.a.run.app/msg/removed-api/
Query params are not handled but can probably punt since we have control of the short urls:
https://pr465-57420cb---lit-dev-5ftespv5na-uc.a.run.app/msg/removed-api?mods=doesquerywork
Is there a way to test the redirect so we don't accidentally break a link. For example updating a content heading could change the heading id and break the fragment redirect.
| path = path.replace(/\/+$/, '/'); | ||
| } | ||
| path = pageRedirects.get(path) ?? path; | ||
| if (path !== ctx.path) { |
There was a problem hiding this comment.
Elegant!
Especially like how this check handles the three different 301 cases.
There was a problem hiding this comment.
Nice, but... can we add a comment? :)
There was a problem hiding this comment.
Added a comment about how it's to flatten redirect chains so we don't actually serve a chain of them.
|
Looks good to me. I hadn't though of the redirect testing that @AndrewJakubowicz mentioned, but I think we'll (at a minimum) need to add comments to each of the docs sections where these links land, so we don't accidentally break the intended association between the current URL and the answer we're looking for (for example, when refactoring one section into two). I opened #467 for that, because it doesn't seem like it needs to be part of this PR. |
| path = path.replace(/\/+$/, '/'); | ||
| } | ||
| path = pageRedirects.get(path) ?? path; | ||
| if (path !== ctx.path) { |
There was a problem hiding this comment.
Nice, but... can we add a comment? :)
Good idea. It would be nice if we could use the link checker we already have for this, but it doesn't support checking for anchors (see stevenvachon/broken-link-checker#108 -- understandable since it would require DOM parsing). I've written a little custom script to do this, will send it in a followup PR.
I think a checker is worthwhile, and then we won't need any comments since CI will break. |
Adds a See https://lit.dev/msg/<code> for more information. string to all warnings logged by Lit's dev mode. Half of #2078. Other half is at lit/lit.dev#465.
Done in #468 |
|
Link checker is 🔥. I'd still like to add comments to at least the sections in the main docs, because those sections serve multiple purposes. We could (and probably should) add a more specific section on class field shadowing, say, or change-during-update, but in doing so we probably wouldn't remove the existing anchor, so the link wouldn't break, even though the relevant content might have moved to another section. |
Adds a
https://lit.dev/msg/<code>redirect for all warnings logged by Lit's dev mode.Half of lit/lit#2078. Other half is at lit/lit#2119.