Add stripExtensions option to determine which extensions get removed.#247
Conversation
Codecov Report
|
amosyuen
force-pushed
the
master
branch
3 times, most recently
from
785db33 to
d85c119
Compare
| [ | ||
| "module-resolver", | ||
| { | ||
| "extensions": [".js", ".jsx", ".es", ".es6", ".mjs"] |
There was a problem hiding this comment.
shouldn't this be stripExtensions? ;)
There was a problem hiding this comment.
Yep, copy and paste error
| } | ||
|
|
||
| export function isRelativePath(nodePath) { | ||
| return nodePath.match(/\.?\.\//); |
There was a problem hiding this comment.
Should we start the regex with ^ to make sure we match only the beginning?
| return name; | ||
| export function stripExtension(modulePath, stripExtensions) { | ||
| let name = path.basename(modulePath); | ||
| stripExtensions.some((extension) => { |
There was a problem hiding this comment.
nit: I'd prefer a reduce function in this case, but it seems ok :)
There was a problem hiding this comment.
Not sure why a reduce is preferred unless you want to strip multiple extensions, but I feel like that could easily behave differently than people expect. Simpler to strip the first matched extension.
There was a problem hiding this comment.
Oh you're right. You only need to do the operation once. My bad.
|
Hmm it seems like this branch is doing a bunch of other stuff other than adding the option (e.g. removing the I'm especially worried about "with the plugin applied twice" test being removed - does this mean the changes broke the test? If so, then it would be better to at least document the new behavior instead of removing the test, and also at this point we're dealing with a breaking change (if this is the case). I suggest splitting the branch into smaller ones, because I'm afraid that too much is happening here at once. |
amosyuen
force-pushed
the
master
branch
2 times, most recently
from
9bc08ac to
72f7667
Compare
|
I moved the relative path check fix to another branch. That also lets us keep the broken test. |
|
Sorry for the silence, I'll take look at the branch in at most two days. Thanks for your patience! |
|
@fatfisz Poke? |
|
@amosyuen Really sorry for the delay, I'm reading the code right now. |
|
@amosyuen Ok, everything's much clearer now! LGTM |
3 participants
Add stripExtensions option to determine which extensions get removed.
Also process relative paths so that extensions get resolved.