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.

Sign up for GitHub

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

Jump to bottom

Update style-loader and mini-css-extract-plugin #9451

Merged
merged 6 commits into from Dec 10, 2020
Merged

Update style-loader and mini-css-extract-plugin #9451

merged 6 commits into from Dec 10, 2020

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Dec 10, 2020

References

Addresses part of #9297

Follow-up to #9364

Code changes

Dependency update.

User-facing changes

None

Backwards-incompatible changes

None

@jtpio jtpio added this to the 3.0 milestone Dec 10, 2020
@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jtpio
Copy link
Member Author

jtpio commented Dec 10, 2020

Looks like a breaking change to fix:

image

@jtpio
Copy link
Member Author

jtpio commented Dec 10, 2020

Documented in the changelog:

https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/CHANGELOG.md#-potential-breaking-change

Just remove them from your configuration(s). We did this because many developers started making these mistakes.

@jasongrout
Copy link
Contributor

It's too bad the nbconvert-css index.css suffers from the same problems we were seeing in css-loader elsewhere - that the css is duplicated many times. For example, it seems that Lumino widget css is duplicated 228 times and Blueprint css is duplicated 101 times. I don't think that is in scope to fix here, but maybe something to think about for the future.

@jasongrout
Deduplicate nbconvert-css css.
617ed4a 
css-loader will duplicate CSS, leading to, for example over 200 copies of the Lumino widget css, over 100 copies of the blueprint css, etc. This uses our new convention for importing package style files as js modules so that webpack does the css duplication for us.
@jasongrout
Copy link
Contributor

It's too bad the nbconvert-css index.css suffers from the same problems we were seeing in css-loader elsewhere - that the css is duplicated many times. For example, it seems that Lumino widget css is duplicated 228 times and Blueprint css is duplicated 101 times. I don't think that is in scope to fix here, but maybe something to think about for the future.

Actually, it looks like the change might be easy. Can I push to your branch?

@jtpio
Copy link
Member Author

jtpio commented Dec 10, 2020

Can I push to your branch?

Sure thing!

@jasongrout
Copy link
Contributor

jasongrout commented Dec 10, 2020 •
edited

The old index.css file is 1,146,862 lines and 46M. The new one is 13,423 lines and 522K. So about 1% the size.

@jasongrout
Copy link
Contributor

Note that our current published nbconvert-css package index.css is 13046 lines, 503K, so as expected, the deduplication was happening before. So this new commit is needed to make sure there is not a regression, and to bring things down to about the same size.

@github-actions github-actions bot added Design System CSS tag:CSS For general CSS related issues and pecadilloes labels Dec 10, 2020
@jasongrout
Ignore linting on the nbconvert raw.js source file.
65eb340 
Our linting is set up to assume every package is TypeScript, but there is no need for nbconvert-css to be typescript - the js file is simply standing in for a css file. Ignoring it avoids the lint errors stemming from this not being a typescript package.
jasongrout
jasongrout approved these changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@jasongrout jasongrout left a comment

Choose a reason for hiding this comment

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

I approve @jtpio's changes.

@jasongrout
Copy link
Contributor

@jtpio - if my changes look good to you, feel free to merge this.

@jtpio jtpio marked this pull request as ready for review December 10, 2020 19:53
@jtpio
Copy link
Member Author

jtpio commented Dec 10, 2020

Thanks Jason for the extra fixes!

@jtpio jtpio merged commit dbeb1d8 into jupyterlab:master Dec 10, 2020
@jtpio jtpio deleted the update-deps branch December 10, 2020 19:56
@jtpio jtpio mentioned this pull request Dec 10, 2020
Closed
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jun 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jasongrout jasongrout jasongrout approved these changes

Assignees
No one assigned
Labels
Design System CSS pkg:extensionmanager pkg:services status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:CSS For general CSS related issues and pecadilloes tag:Examples
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jtpio @jasongrout