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

7.22.0 broke Javascript for me: Uncaught ReferenceError: bootstrap is not defined #2612

Closed
jrochkind opened this issue Jan 19, 2022 · 14 comments · Fixed by #2638
Closed

7.22.0 broke Javascript for me: Uncaught ReferenceError: bootstrap is not defined #2612

jrochkind opened this issue Jan 19, 2022 · 14 comments · Fixed by #2638
Labels
Release-7.x Applies to the 7.x release line

Comments

@jrochkind
Copy link
Member

jrochkind commented Jan 19, 2022

Upgrading from Blacklight 7.21.0 to 7.22.0 broke some javascript for me. Specifically the facet "more" link. After upgrade to 7.22.0, clicking on facet "more" link does nothing, and has this JS error in console:

blacklight.source.js:452 Uncaught ReferenceError: bootstrap is not defined
    at Object.Blacklight.modal.show (blacklight.source.js:452)
    at Object.Blacklight.modal.receiveAjax (blacklight.source.js:388)
    at fire (jquery3.source.js:3496)
    at Object.fireWith [as resolveWith] (jquery3.source.js:3626)
    at done (jquery3.source.js:9786)
    at XMLHttpRequest.<anonymous> (jquery3.source.js:10047)

Of course, I do not expect breakages on a minor version update.

Rails 6.1.4.4

I still include Blacklight js, as well as bootstrap, via sprockets. In my sprockets application.js, I have:

//= require popper
//= require bootstrap-sprockets
//= require blacklight/blacklight

I am using bootstrap-4.6.1 via the ruby gem.

I wonder if this regression is only for sprockets users like me, is maybe why other developers didn't notice? Or is it related to being a bootstrap 4 user, and updates for bootstrap 5?

I'm curious how this regression escaped CI though; does CI not test with sprockets and/or bootstrap 4 anymore?

If I have a chance I'll try to investigate/debug more, but for now just establishing the bug report.

@jrochkind
Copy link
Member Author

jrochkind commented Jan 19, 2022

OK, interesting, I tried to reproduce on a fresh blank blacklight app following quickstart.

It initially did not reproduce. Trying to compare to see what is differnet, I saw that my app had in it's sprockets.application.js:

//= require bootstrap-sprockets

But the quickstart empty blacklight app had just:

//= require bootstrap

Changing the quickstart empty app to //= require bootstrap-sprockets reproduces with Blacklight 7.22.0 again. But not with 7.21.0.

Indeed, in my own app, it APPEARS to work again in BL 7.22.0 if I change require bootstrap-sprockets to require bootstrap.

That is, require bootstrap-sprockets seems to work fine with Blacklight 7.21.0, but not with Blacklight 7.22.0.

I don't understand what the difference is; or where I would have gotten bootstrap-sprockets from in the first place (a past blacklight installer that did it that way?).

The boostrap readme suggests that:

While bootstrap-sprockets provides individual Bootstrap components for ease of debugging, you may alternatively require the concatenated bootstrap for faster compilation

Why would I no longer be able to use require bootstrap-sprockets (individual bootstrap components for ease of debugging) in Blacklight 7.22?

Curious if anyone has any clues here.

It still seems regrettable that a minor BL release had this regression. Especially if require bootstrap-sprockets came from a past Blacklight installer, but I don't know if it did.

@jcoyne
Copy link
Member

jcoyne commented Jan 19, 2022

I think bootstrap-sprockets is from the bootstrap-sass gem. https://github.com/twbs/bootstrap-sass This gem is for Bootstrap 3. I don't know how or why you would be using that with any 7.x version of Blacklight.

@jrochkind
Copy link
Member Author

jrochkind commented Jan 19, 2022

@jcoyne, Thanks, sorry, I should have been more clear and included a link to what I discovered. require bootstrap-sprockets is from the bootstrap 4 or 5 gem, and "provides individual Bootstrap components for ease of debugging". I do not have a bootstrap-sass gem in my dependency tree at all.

The Bootstrap 4.6.1 README says:

Add Bootstrap dependencies and Bootstrap to your application.js:

 //= require jquery3
 //= require popper
 //= require bootstrap-sprockets

While bootstrap-sprockets provides individual Bootstrap components for ease of debugging, you may alternatively require the concatenated bootstrap for faster compilation:

 //= require jquery3
 //= require popper
 //= require bootstrap

The current master branch of the the bootstrap gem for bootstrap 5 has the exact same language.

I do not have a bootstrap-sass gem in my dependency tree at all.

So the require bootstrap-sprockets is the first example in the README for both bootstrap 4 and 5, when distributed as a rubygem. And is documented as "provides individual Bootstrap components for ease of debugging", vs a "concatenated bootstrap for faster compilation."

The only thing that is still mysterious to me is why the (bootstrap-documented) require bootstrap-sprockets worked fine in Blacklight 7.21.0, but stopped working in Blacklight 7.22.0.

@jcoyne
Copy link
Member

jcoyne commented Jan 20, 2022

Could be that #2575 is causing a problem for you. Seems odd that the bootstrap constant isn't exposed though. Are you using babel?

@barmintor
Copy link
Contributor

barmintor commented Jan 20, 2022

I'm looking at the bootstrap.js src vs the bootstrap-sprockets.js src, and... I don't think the latter creates the bootstrap global namespace? It looks like, if you are including one of these files in a non-ES6 module environment, that bootstrap.js sets up $.bootstrap and attaches all the bootstrap classes to it, but the individual files included in bootstrap-sprockets add the bootstrap classes to $ directly

Did we ever actually include bootstrap-sprockets in the generated app?

@barmintor
Copy link
Contributor

barmintor commented Jan 20, 2022

Blacklight.modal.show = function (el) {
  var bsModal = typeof bootstrap === 'undefined' ? Modal : bootstrap.Modal;
  console.log(typeof bootstrap != 'undefined' ? 'bootstrap' : 'bootstrap-sprockets');
  if (bsModal.VERSION >= "5") {
    bsModal.getOrCreateInstance(el || document.querySelector(Blacklight.modal.modalSelector)).show();
  } else {
    $(el || Blacklight.modal.modalSelector).modal('show');
  }
};

if you swap that implementation in for the modal show function, you can see the console log change back and forth when you require bootstrap vs bootstrap-sprockets.

@barmintor
Copy link
Contributor

Docs in the wiki, as another possible place to address the issue: https://github.com/projectblacklight/blacklight/wiki/Using-Sprockets-to-compile-javascript-assets

@barmintor barmintor added the Release-7.x Applies to the 7.x release line label Jan 20, 2022
@jrochkind
Copy link
Member Author

I am not using babel, just ordinary sprockets.

I guess we could just document that if using sprockets it's incompatible with bootstrap-sprockets, that's one option.

I wish I understand what was going on or why Blacklight 7.22 made a difference. Perhaps it's a bug in sprockets, as the bootstrap gem README suggests they should be semantically interchangeable when but clearly they are not. I wish I understood it enough to report it to sprockets if so.

This Rails JS stuff is such a headache these days.

@jcoyne
Copy link
Member

jcoyne commented Jan 20, 2022

We have been testing using //= require bootstrap, so we felt it was safe to use the global bootstrap variable it declares. We have no tests with //= require bootstrap-sprockets, so we didn't detect that in this configuration the variable isn't present. Since at least 7.0.0 we have been generating //= require bootstrap: https://github.com/projectblacklight/blacklight/blob/v7.0.0/lib/generators/blacklight/assets_generator.rb#L24

@barmintor
Copy link
Contributor

Here's the diff between 7.21.0 and 7.22.0: v7.21.0...v7.22.0

If you scroll down to the diff for app/assets/javascripts/blacklight/blacklight.js, you'll see that because Bootstrap 4 and 5 have different APIs for showing/hiding a modal, that script needs to inspect the Bootstrap version in use. It is expecting that global namespace bootstrap to exist (and it does, if bootstrap.js is what was pulled into the application.js sprockets manifest). This is, for what it's worth, nothing to do with Rails - this is purely an issue with the bootstrap js.

@jcoyne
Copy link
Member

jcoyne commented Feb 9, 2022

It looks to me like the bug here is 1fd592c#diff-433d29062dc674b3503c5f4dac5869c2b40ced2ac97576669a3287dba99f0861R444-R457 It's looking for bootstrap and bootstrap isn't defined if you're using Bootstrap 4 and you are using sprockets. I think we can just add a conditional here to look for Bootstrap if it's Bootstrap 4.

@barmintor
Copy link
Contributor

@jcoyne there's a related issue in #2568

@cbeer
Copy link
Member

cbeer commented Feb 25, 2022

I went looking through the history, and it looks like between Blacklight 6 and 7 we:

  • stopped requiring bootstrap in blacklight ourselves, and
  • generated the bootstrap gem and supporting js/css into the host application (using bootstrap.js, not bootstrap-sprockets.js).

bootstrap-sprockets 4.x injects its classes directly onto window (5.x uses bootstrap instead).

I don't see that Blacklight's ever generated or recommended applications use bootstrap-sprockets, but as we've abdicated responsibility for requiring bootstrap, it seems appropriate to support it 🤷‍♂️

jcoyne added a commit that referenced this issue Feb 25, 2022
Backport #2638: Guard against bootstrap variable not being defined; fixes #2612
@jrochkind
Copy link
Member Author

Note that #2638 that closes this issue closes it by not trying to set up modals at all if bootstrap isn't defined -- for me bootstrap would still not be defined, which would just result in a different error report "Blacklight modals, on for instance facet 'more' link, are broken for me after a minor Blacklight upgrade".

I think the issue is that for whatever, reason starting with Blacklight 7.22.0, if loading bootstrap via sprockets, you can no longer do require bootstrap-sprockets (which at one point the bootstrap docs encouraged), you must do require bootstrap instead. I don't totally understand what's going on, but I'll just switch my app to require bootstrap and call it a day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release-7.x Applies to the 7.x release line
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants