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

The included version of the default LightBox2 backend is triggering dependabot alerts #26

Closed
cd-rite opened this issue Jul 23, 2021 · 10 comments

Comments

@cd-rite
Copy link

cd-rite commented Jul 23, 2021

It seems like the included version of LightBox2 is pointed at a specific, very old commit, which includes an old version of jquery.
The newer versions of LightBox2 use later versions of jquery.

Actually, It looks like it's a git submodule that was committed 7 years ago and has not been updated since....

@jonascj
Copy link
Collaborator

jonascj commented Jul 25, 2021

Fine suggestion. I don't see why we should not update it. It is indeed a submodule and it checks out an old version of the plugin.

@jonascj
Copy link
Collaborator

jonascj commented Jul 25, 2021

I wonder if we should update it now (and forget about again for another 7 years) or if we should make the submodule track HEAD of lightbox2. There are currently no tests which verify that the produced html (with lightbox2 javascript and styles) actually works, i.e. that the images popup when clicked...

@t-b
Copy link

t-b commented Jul 26, 2021

I wonder if we should update it now (and forget about again for another 7 years) or if we should make the submodule track HEAD of lightbox2

I tend to use the former (use a fixed version) in that way you know when a problem suddenly surfaces that it can not be some lightbox update.

@jonascj
Copy link
Collaborator

jonascj commented Jul 26, 2021

Yes, that is the advantage of a fixed version. The advantage of using HEAD of a suitable branch is it will be updated whenever there is a new release. But, as new releases are infrequent we can not count on those keeping lightbox2 up to date anyway.

I will update it to a new fixed version for now.

@cd-rite
Copy link
Author

cd-rite commented Jul 26, 2021

Yeah, it's a tricky issue. Do you have Dependabot alerts set up for this repo? At least then you would be alerted to an issue found in a fixed version, even if you are not tracking HEAD. I have the sphinx build product committed in our repo so we can serve out our docs internally, and that's what triggered Dependabot. I'm not sure if including a submodule the way you are would even trigger it....!

@jonascj
Copy link
Collaborator

jonascj commented Jul 27, 2021

I wonder if we could, or should, use the jQuery which is distributed with sphinx. Currently Sphinx distribute jQuery 3.5.1.

Create a new sphinx project, build the HTML and you'll see _build/html/_static/jquery-3.5.1.js.

phinx-quickstart --project "Sphinx jQuery" --author "J. Query" --quiet
make html

It appears easiest to keep it the way it currently is; sphinx-contrib images distribute its own jquery, but it feels a bit redundant ...

├── _build
│   ├── html
│       ├── genindex.html
│       ├── index.html
│       ├── objects.inv
│       ├── search.html
│       ├── searchindex.js
│       ├── _sources
│       │   └── index.rst.txt
│       └── _static
│           ├── jquery-3.5.1.js
│           ├── jquery.js
│           ├── sphinxcontrib-images
│           │   └── LightBox2
│           │       ├── lightbox2
│           │       │   ├── css
│           │       │   │   └── lightbox.css
│           │       │   ├── img
│           │       │   │   ├── close.png
│           │       │   │   ├── loading.gif
│           │       │   │   ├── next.png
│           │       │   │   └── prev.png
│           │       │   └── js
│           │       │       ├── jquery-1.11.0.min.js
│           │       │       ├── lightbox.min.js
│           │       │       └── lightbox.min.map
│           │       └── lightbox2-customize
│           │           └── jquery-noconflict.js
│           ├── underscore-1.13.1.js
│           └── underscore.js

@jonascj
Copy link
Collaborator

jonascj commented Jul 28, 2021

My tests (July 2021) with sphinx-quickstart --project "Sphinx jQuery" --author "J. Query" --quiet make html show this extension can indeed use the jQuery already included by sphinx.

<script data-url_root="./" id="documentation_options" src="_static/documentation_options.js"></script>
<script src="_static/jquery.js"></script>                                   
<script src="_static/underscore.js"></script>                               
<script src="_static/doctools.js"></script>                                 
<script src="_static/sphinxcontrib-images/LightBox2/lightbox2/js/jquery-1.11.0.min.js"></script>
<script src="_static/sphinxcontrib-images/LightBox2/lightbox2/js/lightbox.min.js"></script>
<script src="_static/sphinxcontrib-images/LightBox2/lightbox2-customize/jquery-noconflict.js"></script>

can be reduced to the following and lightbox2 still works:

<script data-url_root="./" id="documentation_options" src="_static/documentation_options.js"></script>
<script src="_static/jquery.js"></script>                                   
<script src="_static/underscore.js"></script>                               
<script src="_static/doctools.js"></script>                                 
<script src="_static/sphinxcontrib-images/LightBox2/lightbox2/js/lightbox.min.js"></script>

However, there seems to be a proposal to remove jQuery as a dependency for Sphinx: sphinx-doc/sphinx#7405 . So I think it seem safer to ship jQuery with sphinxcontrib-images...

@jonascj
Copy link
Collaborator

jonascj commented Aug 3, 2021

@cd-rite The jquery-1.11.0.min.js which is included with sphinxcontrib-images at present comes from lightbox2/js/jquery-1.11.0.min.js (lightbox2@a534955) but in more recent versions of lightbox2 that file no longer exist.

Now lightbox2 ships with a lightbox2/dist/js/lightbox-plus-jquery.min.js (lightbox2@f90a3b0 a file which combines the lightbox2 javascript and jquery in one file).

Would dependabot still work for you if we started shipping lightbox-plus-jquery.min.js with sphinxcontrib-images?

@cd-rite
Copy link
Author

cd-rite commented Aug 5, 2021

@jonascj I think that should work! Thanks!

@jonascj
Copy link
Collaborator

jonascj commented Aug 9, 2021

0.9.4 released with the updated version of Lightbox2 and hence jQuery.

@jonascj jonascj closed this as completed Aug 9, 2021
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

No branches or pull requests

3 participants