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

Bump css-loader #20654

Closed
wants to merge 1 commit into from
Closed

Bump css-loader #20654

wants to merge 1 commit into from

Conversation

item4
Copy link
Contributor

@item4 item4 commented Jan 16, 2020

Description

This PR bump version of css-loader of gatsby for fix broken escaping and spaces in css

Documentation

nothing, just bump css-loader and change configration for newer version

Related Issues

@item4 item4 requested a review from a team as a code owner January 16, 2020 23:04
@pvdz
Copy link
Contributor

pvdz commented Jan 17, 2020

Did you run yarn after bumping? I'm expecting a bunch of yarn.lock updates

I'm not comfortable with the webpack change so I'm leaving that to somebody else to review (@wardpeet?)

modules: {
mode: `local`,
localIdentName: `[name]--[local]--[hash:base64:5]`,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes are all accurate for css-loader@3. E.g. there is no /locals file anymore, camelCase was changed to localsConvention, modules is setup correctly.

One thing I'm seeing here, that's old behavior but something that we could improve is to change the localIdentName to just a hash when we're building for production (unless that's already done via some other mechanism). I'm not arguing for that to be in this PR, just stating so we can track it.

@item4
Copy link
Contributor Author

item4 commented Jan 20, 2020

@pvdz I just uploaded now

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Sadly we cannot upgrade to this version as it requires node 8.9.0. Gatsby requires node 8.0.0 to run.

also css-loader version 2 & 3 have some internal behaviour changes that cannot be fixed by remapping option key/values so we potentially could break people websites.

see https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md#breaking-changes-1 & https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md#breaking-changes for more info

localIdentName: `[name]--[local]--[hash:base64:5]`,
localsConvention: `dashesOnly`,
modules: {
mode: `local`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mode: `local`,
mode: false,

@wardpeet wardpeet added the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Jan 20, 2020
@wardpeet
Copy link
Contributor

Closing for now as we can't upgrade at this point.

@wardpeet wardpeet closed this Feb 12, 2020
@item4
Copy link
Contributor Author

item4 commented Feb 20, 2020

@wardpeet Can you tell me plan of solving this problem?

@pvdz
Copy link
Contributor

pvdz commented Mar 10, 2020

@item4 we expect to bump to node 10 in about one or two weeks. After that this should be viable, provided the node minversion was the only blocker.
Was there anything relevant that's backwards breaking for Gatsby users? (config wise or whatever)

@item4
Copy link
Contributor Author

item4 commented Mar 17, 2020

@pvdz sorry. I don't know well.

@pvdz
Copy link
Contributor

pvdz commented Mar 23, 2020

Fwiw, we've bumped the node version so we can try this now...

@pvdz pvdz reopened this Mar 23, 2020
@pvdz
Copy link
Contributor

pvdz commented Mar 23, 2020

@item4 can you please update the PR (or close this and create a new one) based on the current master? Since we've updated to Node 10 we can now bump this package. Thanks!

@item4
Copy link
Contributor Author

item4 commented Mar 23, 2020

I got it. I will fix confilct asap.

@item4
Copy link
Contributor Author

item4 commented Mar 23, 2020

@pvdz I resolved merge conflict. but it had some problems yet.

  1. css-loader@3 require newer webpack version
  2. because of 1, i can not make new lock file

So I guess this PR will be fail in CI

@pvdz
Copy link
Contributor

pvdz commented Mar 30, 2020

Ok yeah. Bumping webpack is non-trivial so I don't see that happening right now. Guess I'll close this again, sorry about that. Thanks for trying!

@pvdz pvdz closed this Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change If implemented, this proposed work would break functionality for older versions of Gatsby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gatsby transform font-family in @font-face wrong
4 participants