-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Fix stringifyRequest messing with loader configs #58
Conversation
792b6eb
to
bed252d
Compare
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.
looks good beside a minor issue.
You could add a test with UNC paths if you want to: \\server\share\file.js
test/index.js
Outdated
win && { context: "C:\\path\\to\\thing", request: "C:\\path\\to\\module\\a.js", expected: s("../module/a.js") }, | ||
// If context and request are on different hard drives, the path should not be relative | ||
// @see https://github.com/webpack/loader-utils/pull/14 | ||
win && { context: "D:\\path\\to\\thing", request: "C:\\path\\to\\module\\a.js", expected: s("C:/path/to/module/a.js") }, |
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.
expected: s("C:\\path\\to\\module\\a.js")
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 would make sense to make /
as the only separator for requests. Makes it easier for the future. And /
are also valid on Windows.
What do you think?
test/index.js
Outdated
loaderUtils = require("../"); | ||
|
||
var s = JSON.stringify, | ||
j = path.join; |
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.
j is unused.
Thx for reviewing it. I will add that UNC test case. |
bed252d
to
aa644a5
Compare
README.md
Outdated
loaderUtils.stringifyRequest(this, "C:\\module\\test.js"); | ||
// "\"C:\\module\\test.js\"" (on Windows, in case the module and the request are on different drives) | ||
|
||
loaderUtils.stringifyRequest(this, "\\network-drive\\test.js"); |
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.
"\\\\network-drive\\test.js"
README.md
Outdated
@@ -57,13 +57,41 @@ null -> {} | |||
|
|||
### `stringifyRequest` | |||
|
|||
Makes a request pretty and stringifies it. Absolute paths are replaced with relative ones. | |||
Turns a request into a string that can be used inside a `require()` or `import()` call or an `import` statement. | |||
Use it instead of `JSON.stringify(...)` if you're generating code inside a loader. |
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.
... for a require()
or import
.
README.md
Outdated
``` javascript | ||
loaderUtils.stringifyRequest(this, require.resolve("./test")); | ||
// = "../node_modules/some-loader/lib/test.js" | ||
- resolves module requests into relative requests |
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.
module request = "module/path"
relative request = "./relative/path"
stringifyRequest doesn't resolve one into another. Remove this line.
aa644a5
to
9355c27
Compare
stringifyRequest used to replace all backslashes with slashes inside the whole request, including loader configs. This fix separates first the config from the path before applying path normalizations.
It also adds tests.
#37 #54 #55