Ignore field name mappings in package.json files that are not paths of existing files #46
Conversation
And reflect changes in README.md
| // Not sure why we don't just return the full path? Why strip it? | ||
| return doneCallback( | ||
| undefined, | ||
| Filesystem.removeExtension(mainFieldMappedFile) |
There was a problem hiding this comment.
As the comment above asks, why are we actually removing the extension?
There was a problem hiding this comment.
IIRC I added this comment some time ago when doing the initial work on the async version. @Jontem do you remember why the stripping is done? Or do you also think we can just return the raw filename?
There was a problem hiding this comment.
The code has been rewritten many times since i looked at it and i don't recall the reason.
There was a problem hiding this comment.
@christoffer I think you can go ahead and remove the stripping of the extension.
There was a problem hiding this comment.
@jonaskello Hi Jonas. Sorry for the delay, PTOs and what-not came in the way.
Would you be OK if we remove the extension in a separate diff? I feel that it's unrelated to the change I'm introducing here. And since it might have side effects, it would be nice to keep that change separate.
There was a problem hiding this comment.
My original comment was probably unclear. I was only asking out of curiosity, not as a way to ask if I should remove it now :)
There was a problem hiding this comment.
@christoffer Sure, doing it in a separate diff makes sense.
| } else { | ||
| // This is async code, we need to return in an else-branch otherwise the code still falls through and keeps recursing. | ||
| // While this might work in general, libraries that use neo-async like Webpack will actually not allow you to call the same callback twice. | ||
| // An example of where this caused issues: https://github.com/dividab/tsconfig-paths-webpack-plugin/issues/11 |
There was a problem hiding this comment.
Should we keep this comment somewhere or is it no longer relevant?
There was a problem hiding this comment.
Good point, I'll include it at the bottom return statement. Thanks for pointing it out!
| ); | ||
| } | ||
|
|
||
| // This is async code, we need to return unconditionally, otherwise the code still falls |
There was a problem hiding this comment.
@jonaskello Moved it here, let me know what you thing of the formulation. Changed "return in an else-branch otherwise" -> "return unconditionally, otherwise"
|
Released in 3.4.1. |
|
@christoffer Since tsconfig-paths-webpack-plugin depends on |
|
Nope this should be good.
Thanks for the merge!
/Christoffer
…On Sun, Jun 24, 2018, 14:41 Jonas Kello ***@***.***> wrote:
@christoffer <https://github.com/christoffer> Since
tsconfig-paths-webpack-plugin depends on ^3.4.0 of this package I guess
it will pick 3.4.1 now and we won't need to republish the plugin package
for you to get these changes?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#46 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJPwThlKg501CCJOzA-d421S_EQn-rkks5t_4kIgaJpZM4UmOuP>
.
|
Context
Unfortunately it seems like the the change I introduced in #45 wasn't complete.
The
browserfield supports objects as well as strings to allow more specific overrides than a single file: unofficial, but most prominent, spec. I wasn't aware of this, and thus didn't account for this behavior in #45. Currently the plugin will blow up when encountering an "advanced" (spec terminology) mapping.It seems like Webpack supports this spec: https://github.com/christoffer/webpack-resolution-example.
This PR is a suggestion for how to improve the current behavior so that it doesn't blow up when encountering a "bad" mapping, but it's not an implementation of the full spec (see below).
It (silently) ignores mappings that are not a straight file mapping of the main file, i.e. "advanced" mappings. This is obviously far from ideal, but at least it's (arguably) not a digression from the current (and pre-#45) behavior.
With this change, the package.json field names should be accepted only if:
A) they exist
B) they map to an existing file
I'll open an Issue for supporting the full browser spec to get parity with Webpack resolution behavior.
Implementation
While the core of the implementation is as simple as "if
&& is a string && that file exists", the changes required to implement that were a little more involved due to the async file checking. I also took the opportunity to refactor a little bit to get consistent APIs between the sync and async version.