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

feat: add fallback if custom getLocalIdent returns null #1193

Conversation

tvsbrent
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

With the release of the 4.x version of the css-loader, the behavior that allowed a getLocalIdent function to return a falsy value to fallback to the default getLocalIdent function was lost. That original behavior was introduced in this pull request.

This PR reintroduces that behavior, but instead requires the function to return null or undefined.

Breaking Changes

n/a

Additional Info

Fallback to default `getLocalIdent` function if the `getLocalIdent` function
provided in the module options returns a `null`/`undefined`.
@jsf-clabot
Copy link

jsf-clabot commented Sep 22, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #1193 into master will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
+ Coverage   99.24%   99.40%   +0.15%     
==========================================
  Files          10       10              
  Lines         666      670       +4     
  Branches      212      213       +1     
==========================================
+ Hits          661      666       +5     
+ Misses          5        4       -1     
Impacted Files Coverage Δ
src/utils.js 98.69% <100.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df111b8...53d88c1. Read the comment docs.

src/utils.js Outdated

// A null/undefined value signals that we should invoke the default
// getLocalIdent method.
if (localIdent == null && getLocalIdent !== defaultGetLocalIdent) {
Copy link
Member

Choose a reason for hiding this comment

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

Why we need getLocalIdent !== defaultGetLocalIdent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the situation where there wasn't a custom getLocalIdent function defined and the default function returned null or undefined for some reason, I didn't want to run the default again. It wouldn't hurt anything to do so, but it also wouldn't help anything either! ;)

Copy link
Member

Choose a reason for hiding this comment

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

I think it is bad logic, we don't need getLocalIdent !== defaultGetLocalIdent, you can't use defaultGetLocalIdent in own code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, at line 140 in the utils.js, the getLocalIdent property defaults to defaultGetLocalIdent function. So if a user doesn't provide a function, they are implicitly using the default function. The intent for the !== defaultGetLocalIdent check is to handle the situation where the user didn't provide a getLocalIdent function and defaultGetLocalIdent returned a null or undefined.

That may never happen, as there may already be safeguards in place to keep that from happening - and even if it did run the same function twice, it probably wouldn't be a big deal - so that check can be removed.

README.md Outdated
@@ -848,6 +848,8 @@ Default: `undefined`

Allows to specify a function to generate the classname.
By default we use built-in function to generate a classname.
If the custom function returns `null` or `undefined`, we fall back to the
Copy link
Member

Choose a reason for hiding this comment

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

fallback 😄

@tvsbrent
Copy link
Contributor Author

Sorry, I had to fix a commit subject to correct the lint error.

@alexander-akait alexander-akait merged commit 0f95841 into webpack-contrib:master Sep 23, 2020
@alexander-akait
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants