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

get bower dependencies from npm #6213

Closed
wants to merge 1 commit into from
Closed

Conversation

minrk
Copy link
Member

@minrk minrk commented Oct 25, 2021

Avoids dependency on deprecated bower package manager.

This is the same strategy adopted some time ago by JupyterHub, which also used Bower. Rather than making significant changes to how everything is built, which caused problems during the webpack, this only changes how the dependencies are downloaded. A bower-lite script copies dependencies declared in package.json from node_modules to notebook/static/components to match the previous installation location.

Most packages are identical, but a couple of packages have slightly different layouts (jquery-ui, react, xterm). It's unclear if these relative URLs should be considered public APIs, in which case we can make do more detailed renames to match everything exactly.

Related to #6210

@minrk minrk force-pushed the bower-lite branch 4 times, most recently from 3fe19a9 to d03d798 Compare October 25, 2021 11:10
a couple of packages have different layouts (jquery-ui, react, xterm), but little is changed

a 'bower-lite' script copies dependencies from node_modules to `static/components` to match previous installation location
@Zsailer
Copy link
Member

Zsailer commented Oct 25, 2021

Thanks, @minrk! We'll review this in Wednesday's notebook meeting. 😎

@minrk
Copy link
Member Author

minrk commented Oct 25, 2021

I think there's a real test failure, presumably related to a url somewhere or other that needs updating, but I haven't found it. Things seem to work when I test locally.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I'm not qualified to review these changes, notebook builds and appears to run fine.

I think we should move forward with this so we can address #6254, but would like someone with frontend experience to review as well.

@kevin-bates
Copy link
Member

I think there's a real test failure, presumably related to a url somewhere or other that needs updating, but I haven't found it.

@minrk - could you please point out the test failure (tests aren't very stable at the moment and I'm not sure what might be real or fake)? Perhaps someone could then take a look if there's a starting point.

@RRosio
Copy link
Collaborator

RRosio commented Aug 1, 2023

Closing this PR as an issue has been opened for this work on NbClassic, where this could be addressed.

@RRosio RRosio closed this Aug 1, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants