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

Move as much JS as possible from sprockets to vite #1889

Merged
merged 11 commits into from
Oct 20, 2022

Conversation

jrochkind
Copy link
Contributor

@jrochkind jrochkind commented Oct 17, 2022

To be merged into #1882

@jrochkind
Copy link
Contributor Author

jrochkind commented Oct 18, 2022

OK, just keeping notes on a problem I'm running into....

browse_everything can't be moved over from sprockets, it only offers JS via sprockets... it's sprockets JS include also APPEARS to be automatically loading bootstrap JS... such that when I try to move bootstrap JS over to vite, I end up with a double load, which is a JS-file-size problem no matter what, but also makes some aspects of the JS not work right.

samvera/browse-everything#411

@jrochkind
Copy link
Contributor Author

Our sprockets-based stuff right now includes defualt rails-generated "stubs" for ActionCable

Eg https://github.com/sciencehistory/scihist_digicoll/blob/1216313ba4d1737050e689756fdfff15ab543837/app/assets/javascripts/cable.js

This is not a Rails feature we have ever used. I'm going to remove it and not copy it over to vite.

This will make our JS bundle (trivially) smaller, and our general structure simpler. If we ever wanted ActionCable, we'd have to add it back to our vite config, which might be a bit of a pain to figure out how to do, but cross that bridge when we come to it.

@jrochkind jrochkind force-pushed the vite_move_more_js_over branch from 576eaf3 to 973c6c4 Compare October 19, 2022 15:11
With weird workaround to prevent Blacklight sprockets-included JS from triggering strict mode, which breaks current version of blacklight_range_limit JS
Unclear exactly what default is or how to turn off in prod. ElMassimo/vite_ruby#285
@jrochkind
Copy link
Contributor Author

OK at this point in this PR almost all of our JS has been moved over. Some exceptions:

  • blacklight itself -- just haven't bitten that off yet, and probably want to do it when we also move CSS to vite, so blacklight can have both JS and CSS coming from blacklight_frontend NPM package.

  • browse_everything -- no supported mechanism to include its JS but sprockets. This PR moves this to be loaded only on admin pages though, since that's the only place it's used.

  • blacklight_range_limit -- no supported mechanism to include its JS but sprockets.

This PR also improves vite configuration a bit:

  • make sure sourcemaps are on in development autoBuild

  • Tell vite to split out video.js stuff in a separate "chunk" -- turns out the video.js still is huuuugee, this may help with performance, and/or with warnings about big files. We aren't doing anything to load it dynamic on demand or anything, video.js is still statically imported in our JS file on page load, just vite splits it into it's own file.

We also had to put in one weird workaround, to stop the blacklight (sprockets) code from triggering browser JS strict mode, which the blacklight_range_limit (sprockets) code was not compatible with! Once blacklight (sprockets) was FIRST in the sprockets file, it's "use strict" declaration actually worked -- and broke blacklight_range_limit!!

At this point, this is ready to merge into the vite branch -- which I think then can be read to merge into master and deploy actually.

@jrochkind jrochkind marked this pull request as ready for review October 20, 2022 18:49
@jrochkind jrochkind mentioned this pull request Oct 20, 2022
@eddierubeiz eddierubeiz merged commit 759a5b9 into vite Oct 20, 2022
@eddierubeiz eddierubeiz deleted the vite_move_more_js_over branch October 20, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants