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

Adjust versions and arguments so tests pass #3181

Merged
merged 10 commits into from
Oct 18, 2024

Conversation

taylor-steve
Copy link
Contributor

@taylor-steve taylor-steve commented Oct 9, 2024

The goal of this PR is to get the tests passing, so we have a real baseline. This way we can get CI feedback if we break things.

  • Switches from ~ to ^ for bootstrap.
    yarn build:css/rake assets:precompile was failing in some cases because with the current setup the propshaft generator was literally installing bootstrap 4.0.

  • Has a temporary (?) fallback mechanism so that in cases like BL 7.39.0 that do not have a corresponding NPM package it will try and install the latest version for that major release.

  • [Temporary Workaround] Somewhere along the way we (probably me!) broke something related to Bootstrap 4 and bundling. When imported in application.js and bundled via esbuild, we were losing functions like modal in files like zpr_links.js. Long story short our lives would be much easier if we can drop either jquery-serializejson or Bootstrap 4, but I will attempt to review the history and find out where the bundling broke when I return. For now, as a stopgap, I'm including the Bootstrap 4 bundle package in the layout when BS4 is installed.

Somewhere we lost the jQuery integration for BS4 (e.g., modal, tab) in some contexts (e.g., zpr_links.js) when importing BS4 in the app's application.js to be bundled with the default esbuild. It seems to work when included in the layout. Let's see if this passes the test suite.
@taylor-steve taylor-steve changed the title Test using a newer version of Bootstrap 4 Adjust versions and arguments so tests pass Oct 9, 2024
@taylor-steve taylor-steve marked this pull request as ready for review October 9, 2024 23:40
@jcoyne
Copy link
Member

jcoyne commented Oct 10, 2024

@taylor-steve Can these changes be split into separate PRs? These three changes seem like they could stand on their own. I don't see how the Dockerfile changes relate to the tests passing.

@taylor-steve
Copy link
Contributor Author

@jcoyne This is for the propshaft branch and those Dockerfile changes were needed to make the docker build check pass. The team is still debating how to handle rewriting history on the overall branch.

@jcoyne
Copy link
Member

jcoyne commented Oct 15, 2024

@taylor-steve can the changes not relating to propshaft (like .ruby.yml) be made as separate PRs on main? Then you can merge those commits into the propshaft branch once they are accepted. This will cause the least departure from the main branch and make it much easier for people to review the propshaft changes.

@jcoyne
Copy link
Member

jcoyne commented Oct 15, 2024

Can you explain

This is our current workaround for preventing importmap:install from wanting to overwrite our application.js.

Importmap:install will happen well before application.js is written, right?

@taylor-steve
Copy link
Contributor Author

@taylor-steve can the changes not relating to propshaft (like .ruby.yml) be made as separate PRs on main? Then you can merge those commits into the propshaft branch once they are accepted. This will cause the least departure from the main branch and make it much easier for people to review the propshaft changes.

Yes, that makes sense.

Can you explain

This is our current workaround for preventing importmap:install from wanting to overwrite our application.js.

Importmap:install will happen well before application.js is written, right?

The behavior I see is that importmap:install occurs after the Spotlight template.rb steps. It's the other way around during test app/engine cart generation. The Dockerfile uses the template to build. We spent a little time trying to find a better way, but for now this seemed to suit our immediate need.

@jcoyne
Copy link
Member

jcoyne commented Oct 15, 2024

@taylor-steve how about using after_bundle instead? #3192

taylor-steve and others added 4 commits October 16, 2024 16:15
They are unnecessary. The Blacklight frontend package is based on the
actual installed gem version. After the switch to using '^', the highest
allowed version of bootstrap will be installed anyway.
This ensures the importmap generator runs before any of the spotlight generators
@corylown corylown merged commit 0f8716c into migrate-asset-pipeline Oct 18, 2024
6 checks passed
@corylown corylown deleted the bump-bootstrap-version branch October 18, 2024 16:19
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.

3 participants