-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix Prod Build Bug w/ Bootstrap & Updated Dependencies #59
Conversation
Visit the preview URL for this PR (updated for commit 93c2ca2): https://tcl-77-smart-shopping-list--pr59-update-deps-d8hg77f6.web.app (expires Fri, 11 Oct 2024 07:34:02 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b77df24030dca7d8b6561cb24957d2273c5e9d72 |
We were working in office hours to see if we could fix the prod link not working |
…p better, resolve bootstrap and css preprocessor setting for vite compiling of project
…oking for root DOM element and not getting it causing react minification error
…e one the homepage is using has the scss styling and made sure the unauthenticated used it as well
Honestly shoutout to yall @RossaMania for sparking the idea that it was a config issue in vite, @adidalal for the dependency updates and the resolver with the change in imports because it was needed, @alex-andria, @zahrafalak and @eternalmaha for staying after office hours and late to just bounce ideas and problem solve. Oh and Alex for finding the breadcrumb to look at the manual chunking. And in general for bouncing ideas and hearing me out as I tried to figure out where the bootstrap issue started! 🎉✨ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow amazing work! Great investigative skills and putting that research into an actionable and working solution. From my end in local and build preview, bootstrap is working!!
I did notice that there are some design flaws in the item list that if your team decides to fix in a later PR may be a good start.
Great work!
if (id.includes("react-bootstrap") || id.includes("@restart")) { | ||
return "vendor__react-bootstrap"; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WOOOO!! Great job continuing from where we left off Brianna!! I'm amazed by your research skills - as always - and am so happy that you found this fix after reading the Vite and rollup documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You really sprung it into action with finding the manual chucking commented out worked again!
api: "modern-compiler", | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the same changes from Falak's old PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, it was one of the things that cute talks about when using sass and I didn't see it in there when I was writing the resolver and the manual chunks so I did add it as well. I'd have to look at the old PR 🤔 this was branched from the messed up main so it may have been in a PR but didn't merge 🤷🏽♀️
@alex-andria If its the black box, the scrolling with that filter area staying on top was implemented before we had colors and I have dark mode on my browser so I just picked a color that would let me see the that area floated on top while scrolling 😅 it should be changed as we fix the design. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job figuring this out! I know from experience that deploying with Vite can be tricky! It looks like everything is up to date!
Description
As we discovered on Monday the production version of the app was mad 😡. It was working locally but not after getting built somehow. While the build steps were "passing" in Github Actions something was not right.
In office hours we updated some of the dependencies, but the build still wasn't working. Then we looked at how
bootstrap/react-bootstrap
worked withvite
and howsass
needed to be imported. We wrote a resolver in thevite config
file to be able to properly resolve thesass
imports as we found in the documentation. After we got to those steps we did some manual testing of how the application was handling the building of it in themanualChunks
seeing that without that there wasn't an issue with the app but it was saying the minification was a bit large.After finding that the
manual chunking
seemed to be a sticking point, I went through thevite
androllup
documentation to see how it was handled. With the error focusing on aforwardRef
issue I did a bit of googling and it seemed it was something used byreact-bootstrap
and themanual chunking
was chunking parts of thenode_modules
to minify it but was separating parts that thereact-bootstrap
needed in a dependency of it called@restart
I added aguard clause
to themanual chunking
that would bundle thereact-bootstrap
and@restart
into the same group. That for rid of theforwardRef
and I got areact minification #299
that when looking it up said thetarget wasn't a DOM element
it was and changing the script tag in the html todefer
got rid of that.Lastly I noticed there was a duplication in the
AuthnticatedNavBar
and the one the app was using wasn't getting the.scss
applied to that so I did correct that here as well.Acceptance Criteria
Type of Changes
Updates
Before
After
Testing Steps / QA Criteria