feat(hashi-head): validate props.image, add twitter:description #623
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 29892d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/head/index.tsx
Outdated
| key="description" | ||
| /> | ||
| <meta | ||
| // TODO: are we sure we need this? Seems like will fall back to OG |
There was a problem hiding this comment.
@BRKalow wanted to confirm - do we strictly need twitter:description? It seems like it should fall back to og:description (ref: https://developer.twitter.com/en/docs/twitter-for-websites/cards/overview/markup). Wondering whether we should omit it (or perhaps duplicate all such fallback tags with twitter: specific versions, for consistency?)
There was a problem hiding this comment.
I think you should be able to test this both with & without twitter:description using the Twitter card validator. Have you tried it out yet to help confirm the necessity of this tag?
There was a problem hiding this comment.
✅ Great call, and somewhat contrary to Twitter' docs, it does seem necessary (tested in hashicorp/dev-portal#658).
There was a problem hiding this comment.
UGH that's so annoying 😭 but great tests! 🙌🏻 Glad that was fairly straightforward to test.
| * @param string The URL to test | ||
| * @returns true if the URL is absolute, false otherwise | ||
| */ | ||
| function isAbsoluteUrl(string: string): boolean { |
There was a problem hiding this comment.
Pulled in from hashicorp/dev-portal (and elsewhere).
There was a problem hiding this comment.
(Question 🙋) Can some examples of what is and isn't an absolute URL be added to the block comment above this function? That will help make it clear at a glance how this function works. I end up having to look it up every time. 😅
(Future 📌) Tests would be nice as well, but maybe this is a helper we should prioritize moving to @hashicorp/platform-util first? Sounds like it's very widely used!
There was a problem hiding this comment.
✅ Great call! Added some JSDoc @example lines. Let me know if that seems good!
Also very on board with adding this to @hashicorp/platform-util at some point in the near future. Added a task to our platform board: 🎟️ [platform-util] add isAbsoluteUrl, it is widely used and implementation has been pretty static
There was a problem hiding this comment.
These examples look great! Very clear and super helpful. Thank you for adding 💖
packages/head/index.tsx
Outdated
| */ | ||
| if (typeof props.image !== 'undefined' && !isAbsoluteUrl(props.image)) { | ||
| /** | ||
| * TODO: should we consider alternatives to throwing an error here? |
There was a problem hiding this comment.
@BRKalow Forget to mention in review - wanted to get your take on the error approach here. I think the IS_DEV approach could make sense, but would love a second opinion on that (and also a lil curious about whether further tracking, eg in DataDog if available, sounds like something we might want to do... console.error seems like something that might not have significant visibility for us?).
There was a problem hiding this comment.
If our goal is to fix all existing now-incorrect values for props.image, it seems reasonable to me to always throw an error regardless of the NODE_ENV. No production builds will be immediately broken unless the version for this package is upgraded after release right? We'll just start seeing failed Vercel builds in PRs?
There was a problem hiding this comment.
☝️ I think we'd definitely want to always throw an error if we did 100% static builds, but might not be as safe anymore since we don't static render all pages... for ISR pages, I think throwing an error could cause a client-side error, or possibly a 500 for the initial server-side render? Does that sound accurate?
There was a problem hiding this comment.
Hmm, yeah. That makes sense. I wonder if we can test different cases to make sure? But that can definitely be done outside of this PR! We can always update this block in the future if needed. 📌
zchsh
changed the title
🎟️ Asana Task
🔍 Preview Link
This PR adds validation to try to ensure that the
imageprop provided to@hashicorp/react-headis an absolute URL. This is to match the Open Graph protocol, which notes All valid URLs that utilize the https:// or https:// protocols.PR Checklist 🚀
Items in this checklist may not may not apply to your PR, but please consider each item carefully.