design-system icon indicating copy to clipboard operation
design-system copied to clipboard

Fix sidecar in website

Open alex-ju opened this issue 9 months ago • 6 comments

:pushpin: Summary

Fix the sidecar in website by ensuring the layout object exists. While the fix doesn't demystify the root cause it does seem to fix the immediate issue. I suspect a mix of race and frontmatter object override.

To test the fix run ember s --environment=production to get prember to serve static assets. /about/principles is a good page to check the sidecar doesn't render.

:link: External links

Jira ticket: HDS-4770


👀 Component checklist

  • [ ] Percy was checked for any visual regression
  • [ ] A changelog entry was added via Changesets if needed (see templates here)

:speech_balloon: Please consider using conventional comments when reviewing this PR.

alex-ju avatar Apr 10 '25 12:04 alex-ju

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Apr 10, 2025 0:28am
hds-website ✅ Ready (Inspect) Visit Preview Apr 10, 2025 0:28am

vercel[bot] avatar Apr 10 '25 12:04 vercel[bot]

@alex-ju I'm looking at this to see if I remember something more of what I did and why

didoo avatar Apr 15 '25 18:04 didoo

@alex-ju something very weird is happening here.

Compare the source code: modified

With how it's compiled: modified

I need to investigate more...

didoo avatar Apr 15 '25 18:04 didoo

@alex-ju I went back to a previous commit (https://github.com/hashicorp/design-system/pull/2361 and since then the logic has not been changed); then I looked at how the code would be compiled, and from what I see it's compiled as one would expect: modified

So it must be something not related to our code but to the compilation itself. Could it be something introduced with the migration to pnpm? @aklkv can you think of something that could change so drastically the code compilation?

didoo avatar Apr 15 '25 20:04 didoo

@alex-ju I went back to a previous commit (#2361 and since then the logic has not been changed); then I looked at how the code would be compiled, and from what I see it's compiled as one would expect

So it must be something not related to our code but to the compilation itself. Could it be something introduced with the migration to pnpm? @aklkv can you think of something that could change so drastically the code compilation?

The compiled code is the same, from what I've seen, but the computed value for frontmatter is different – I saw the same results in my debugging with the ones you shared above. This makes me think that the issue has to do with various async tasks taking longer or shorter than before (potentially after the pnpm migration), but couldn't get to the bottom of it.

alex-ju avatar Apr 16 '25 09:04 alex-ju

@alex-ju @aklkv I was able to narrow down the PR that introduced this regression and it is this one: https://github.com/hashicorp/design-system/pull/2729

This is the generated code when you check out https://github.com/hashicorp/design-system/pull/2708 which is the previous PR that was committed: modified

as you can see the hasCover/hasSidecar are correctly evaluated as frontmatter?.layout?.cover/sidecar ?? true and then assigned to this.modelCache[path] as variables (o/r)

This instead is the generated code when you check out https://github.com/hashicorp/design-system/pull/2729: modified

As you can see, the logical evaluation has disappeared, and the hasCover/hasSidecar are always assigned as true to the this.modelCache[path] object (and in fact in this screen you see the sidecar appear even if it should not).

I am not sure how to debug more this, probably @aklkv may have some ideas of what may be?

PS for context, if I change this line:

const hasSidecar = frontmatter?.layout?.sidecar ?? true;

to this:

let hasSidecar;
if (frontmatter.layout && frontmatter.layout.sidecar === false) {
  hasSidecar = false;
} else {
  hasSidecar = true;
}

suddenly the compiler changes how the logic is converted to something that "works": modified

So it must be a compilation problem, not a source code problem.

didoo avatar Apr 16 '25 11:04 didoo

I finally had time to poke around, I was able to reproduce in current deployed version but if I check my #2810 website preview it seems to be just fine. I wanted to make sure I was not missing anything

aklkv avatar May 17 '25 11:05 aklkv

@didoo, your hunch was spot on. This type of check:

const hasCover = frontmatter?.layout?.cover ?? true;

was previously affected by a Babel issue. In versions prior to Babel 7.8, the nullish coalescing operator (??) wasn’t fully supported and could be transpiled incorrectly, sometimes behaving like the logical OR operator (||). That means if the left-hand side was false, it would incorrectly fall back to the right-hand side — which is not the expected behavior of ??.

In my branch, I bumped the Babel version (either directly or via a preset upgrade), and the issue resolved itself — everything now behaves correctly.

Gotta love the quirks of transpilers. 😅

aklkv avatar May 17 '25 11:05 aklkv

I’ve bumped babel to the latest version across all packages as a precautionary measure. #2884

aklkv avatar May 18 '25 03:05 aklkv

@didoo, your hunch was spot on. This type of check:

const hasCover = frontmatter?.layout?.cover ?? true;

was previously affected by a Babel issue. In versions prior to Babel 7.8, the nullish coalescing operator (??) wasn’t fully supported and could be transpiled incorrectly, sometimes behaving like the logical OR operator (||). That means if the left-hand side was false, it would incorrectly fall back to the right-hand side — which is not the expected behavior of ??.

In my branch, I bumped the Babel version (either directly or via a preset upgrade), and the issue resolved itself — everything now behaves correctly.

Gotta love the quirks of transpilers. 😅

That is so fascinating! 😯 🤯

/cc @alex-ju let's wait for #2884 to be merged, and then we can see if some version of this PR changes are still needed (probably not anymore?)

didoo avatar May 19 '25 11:05 didoo

@alex-ju @aklkv the website issue with the sidenav has been fixed by #2884 (thanks @aklkv for the extra investigation) so we can close this PR (and the associated ticket): https://hds-website-git-chore-babel-hashicorp.vercel.app/about/principles

didoo avatar May 19 '25 20:05 didoo