-
-
Notifications
You must be signed in to change notification settings - Fork 605
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
feat: add fallback if custom getLocalIdent returns null #1193
Conversation
Fallback to default `getLocalIdent` function if the `getLocalIdent` function provided in the module options returns a `null`/`undefined`.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
src/utils.js
Outdated
|
||
// A null/undefined value signals that we should invoke the default | ||
// getLocalIdent method. | ||
if (localIdent == null && getLocalIdent !== defaultGetLocalIdent) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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! ;)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fallback
😄
b406380
to
53d88c1
Compare
Sorry, I had to fix a commit subject to correct the lint error. |
Thanks! |
This PR contains a:
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 defaultgetLocalIdent
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
orundefined
.Breaking Changes
n/a
Additional Info