Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
damithc opened this issue Apr 29, 2024 · 12 comments · Fixed by #2542
Closed

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

damithc opened this issue Apr 29, 2024 · 12 comments · Fixed by #2542
Labels
f-Icons Glyphicons etc p.High To be done in the next few releases s.ToInvestigate

Comments

@damithc
Copy link
Contributor

damithc commented Apr 29, 2024

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
Copy link
Contributor

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: #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.

@kaixin-hc kaixin-hc added p.High To be done in the next few releases f-Icons Glyphicons etc labels Apr 30, 2024
@yiwen101
Copy link
Contributor

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

@kaixin-hc
Copy link
Contributor

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

@damithc
Copy link
Contributor Author

damithc commented Apr 30, 2024

@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
Copy link

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 
Screenshot 2024-05-01 at 2 08 07 AM

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
Copy link
Contributor

yiwen101 commented Apr 30, 2024

@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
Copy link
Contributor

yiwen101 commented Apr 30, 2024

Strangely, I still see the error even if I do this
Screenshot 2024-05-01 at 03 35 28
Screenshot 2024-05-01 at 03 35 12

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?
Screenshot 2024-05-01 at 03 38 04

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

@tlylt
Copy link
Contributor

tlylt commented May 1, 2024

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

@kaixin-hc
Copy link
Contributor

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
Copy link
Contributor

yiwen101 commented May 1, 2024

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

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.
Screenshot 2024-05-01 at 12 52 06

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.
Screenshot 2024-05-01 at 12 52 48

@EltonGohJH
Copy link
Contributor

EltonGohJH commented May 1, 2024

EltonGohJH/2024@c6d2191

image
image

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f-Icons Glyphicons etc p.High To be done in the next few releases s.ToInvestigate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants