Fix getmetadata invalid site url #1362
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 564 Passed, 42 Skipped, 1m 26.42s Total Time |
| const canonicalUrl = (() => { | ||
| if (!props.site.url) return props.page.permalink | ||
| try { | ||
| const siteUrl = | ||
| props.site.url.startsWith("http://") || | ||
| props.site.url.startsWith("https://") | ||
| ? props.site.url | ||
| : `https://${props.site.url}` | ||
| return new URL(props.page.permalink, siteUrl).toString() | ||
| } catch { | ||
| return props.page.permalink | ||
| } | ||
| })() |
There was a problem hiding this comment.
actually i disagree with this approach; i'd rather we just throw an error here so that we can guarantee that the site.url in our db is a well-formed url.
if we go with this approach, we should also disallow http://
There was a problem hiding this comment.
I think @seaerchin has a good point, we should rather fix and be opinionated about the expected input as the schema is the contract to be followed/adhered to. When it does not match the expected input, it should error out
Else, we will end up with schemas of site that vary between each other.
As per URL standards it should be https:// or http://. But for Isomer, we will strictly disallow http, so it should throw as well for http
There was a problem hiding this comment.
@seaerchin @harishv7 updated!
…proved clarity and error handling
| @@ -58,11 +58,25 @@ const getMetaImage = (props: IsomerPageSchemaType) => { | |||
| } | |||
| } | |||
|
|
|||
| const getCanonicalUrl = (props: IsomerPageSchemaType) => { | |||
| if (!props.site.url) return props.page.permalink | |||
There was a problem hiding this comment.
Is it guaranteed that either site.url or page.permalink will always be present
There was a problem hiding this comment.
page.permalink is stored in the page as part of the JSON for every blob. It should unless someone intentionally remove it directly (which should not happen)
| const getCanonicalUrl = (props: IsomerPageSchemaType) => { | ||
| if (!props.site.url) return props.page.permalink | ||
|
|
||
| if (!props.site.url.startsWith("https://")) { |
There was a problem hiding this comment.
i think can add a comment explaining the decision made that we want to explicitly throw errors to standardise the schema for site config urls
Problem
We had an issue last week where
urldoesn't havehttps://and thus calling site build to fail with an unknown errorSolution
Breaking Changes
Bug Fixes:
try/catchfor better error handlinghttp://orhttps://and auto append if missing