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

Improve javascript #15175

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Conversation

uberbrady
Copy link
Collaborator

My main goal here was to get our build-cycle to use require() statements rather than concatenating files together. This puts us on a path to use Laravel Vite instead of Webpack to generate our assets. This also makes the webpack config file easier to read, and has us making library-level changes more at the snipeit.js level. This process was made much easier by the fact that the bulk of everything we do is powered by jQuery - so once you have a working jQuery object, everything just attaches itself to that.

I was able to get rid of the 'stub' bootstrap.js file (that file means "Bootstrapping the JavaScript" - no relation to bootstrap, the CSS framework). I just included the main contents at the top of snipeit.js.

I was able to remove an entire step from the build-cycle, which is a bonus. I also added in SourceMaps, which should make troubleshooting our Javascript much easier when the problem is somewhere within our all.js file.

I could keep going but I didn't want to paint myself into a corner, and this is already a little easier to understand than what we had going before.

I haven't clicked around everything in the app, but I did find places where the new generated all.js file wasn't working, but was able to fix those. I imagine I probably missed some more things. I tried to leave comments in the snipeit.js file to explain what files are being included, and why.

There are some files that get generated that aren't included in .git - I decided not to add them here, since we tend to build JS/CSS assets before releases, they'll show up again.

But this is at least a start.

@uberbrady uberbrady marked this pull request as ready for review September 4, 2024 13:44
@uberbrady uberbrady requested a review from snipe as a code owner September 4, 2024 13:44
@snipe
Copy link
Owner

snipe commented Jan 16, 2025

@uberbrady Are we still looking to take this? We have conflicts, but only in the build stuff, which shouldn't matter, I wouldn't think

@uberbrady
Copy link
Collaborator Author

Yes, I'm definitely still interested in this. I'll see if I can get the conflicts resolved, at least.

@uberbrady
Copy link
Collaborator Author

Yay! After pushing up the build artifacts, the tests seem to pass OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants