CS2103 website build fails due to missing bootstrap-icons.css

MarkBind / markbind

MarkBind is a tool for generating content-heavy websites from source files in Markdown format

https://markbind.org/

MIT License

134 stars 123 forks source link

CS2103 website build fails due to missing bootstrap-icons.css #2538

Closed damithc closed 2 months ago

damithc commented 2 months ago

v5.5.0

CS2103 website doesn't work anymore after upgrading to v5.5.0 The error I get when running markbind serve -o is the below:

warn:  message=Cannot find module 'bootstrap-icons/font/bootstrap-icons.css'
...

kaixin-hc commented 2 months ago

Seems it was added to the wrong package lock and we missed it. See similar PR: #1681 and look at the files changed in the PR I just made: https://github.com/MarkBind/markbind/pull/2539

Locally, it runs fine - I assume because of hoisting? It also works for the MarkBind website for some reason - and most CI check passes.

https://www.jonathancreamer.com/inside-the-pain-of-monorepos-and-hoisting/ talks about some of the edge cases that can come up in a monorepo. I haven't quite worked out how this one happened where it throws a fit on some sites but not others, but its likely for a similar reason.

Tagging @yiwen101 because he might be curious about this.

yiwen101 commented 2 months ago

Sorry for causing this issue due to my neglect. @kaixin-hc Thank you so much for your quick fix and investigation

kaixin-hc commented 2 months ago

@yiwen101 on't worry! None of us caught it either in review, such things happen

damithc commented 2 months ago

@yiwen101 on't worry! None of us caught it either in review, such things happen

Yup. these things happen. While we should try to minimize, it's not always feasible to do so. So, what matters is how quickly we can respond to such cases when discovered.

On a related note, it might be a good idea to smoke test on the CS2103 website before we do a release, or before merging a major feature. If it can survive the CS2103 website, chances are it can survive anything :-D

ckcherry23 commented 2 months ago

Seems like the CS3282 website deployment is still having some issues with the new version due to an undefined "path" argument.

uncaughtException: The "path" argument must be of type string. Received undefined

CI run: https://github.com/nus-cs3281/2024/actions/runs/8899519872/job/24439067023

On a related note, it would be nice if the CI status fails when the deployment has an error, for better DX.

yiwen101 commented 2 months ago

@ckcherry23 Thank you for raising this issue up, and the suggestion on better CI. A quick investigation shows that this problem is first observable after this depend-bot update pr that bumps gp-pages from 2.2.0 to 5.0.0.

yiwen101 commented 2 months ago

Strangely, I still see the error even if I do this

I also have no idea where the argument "path" in the error message comes from. It must not the argument we pass to it right?

I wonder if there is a better fix than reverting the version update.

tlylt commented 2 months ago

On a brief look, is the error because of Vue hydration issue that breaks the page? Maybe @yiwen101 can confirm

kaixin-hc commented 2 months ago

Interesting... @EltonGohJH has been looking into this, we think it might be the 3.0.0 update of gh-pages that moves the cache directory default location and also allows you to manually specify a cache directory. It might be that it is required

yiwen101 commented 2 months ago

On a brief look, is the error because of Vue hydration issue that breaks the page? Maybe @yiwen101 can confirm

I do not think the missing tbody issue is related to the "path is undefined" problem we are facing, although there is indeed table without tbody spotted here.

But it does not seem to be causing hydration in the current deployed site.

Interestingly, I also noticed that when I checkout to ba0b92d28, this code that has caused hydration in the past also does not cause hydration locally on my device now.

EltonGohJH commented 2 months ago

https://github.com/EltonGohJH/2024/commit/c6d21912e250b410294effe42f2faf190f9139c6

The problem seems that when running the github action it fails to locate the .cache folder. When I set it manually in my fork it works.

So, I created a PR to set it to a location if it is github action.

kaixin-hc commented 2 months ago

Interestingly, I also noticed that when I checkout to ba0b92d28, this code that has caused hydration in the past also does not cause hydration locally on my device now.

Very interesting! Hmm... if its now so robust even that doesn't throw an error, we can consider if we need to remove it later. But better safe than sorry. Anyway, JIC I've pushed the proper vue hydration to the 2024 master, doesn't hurt anything