-
-
Notifications
You must be signed in to change notification settings - Fork 380
Improve getFilenameFromUrl
to work with proxy requests
#80
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
Conversation
Sorry, I've reverted your commit (b05cd04) because it broke a lot of code out there (#79 #81 #82). Could you tell me what the purpose of b05cd04 was? I'm not sure if I'm understanding this PR correctly, but it looks to me that it does not cover the old implementation. I won't merge this PR until its purpose is clear to me. Sorry again, that we don't have any tests yet. It makes contributing a lot harder. It's certainly not your fault. |
I use webpack-dev-server and express, and SwitchOmega Plugin to proxy requests to the localhost. so that the request hostname won't match because I wrote this gist to explain the case: |
getFilenameFromUrl
to work with proxy requests
@ijse, is it not possible to exclude dev-server from the proxy? Also see webpack/webpack-dev-server#600 (comment). Not excluding the dev-server seems to result in more issues after this. |
I recently added extensive tests for 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.
Could you fix the merge conflict, and make sure it passes the new tests (npm test
)?
Conflicts: middleware.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.
Looks very good, I like how it also makes the code somewhat easier to read.
If you fix the issues with RegExp
, I'll merge it :).
} | ||
|
||
// publicPath is not in url, so it should fail | ||
if(publicPath && localPrefix.hostname === urlObject.hostname && !new RegExp('^' + publicPath).test(url)) { |
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.
Inserting publicPath
into RegExp
can lead to unexpected errors, since the string will be interpreted as regex.
Maybe use lastIndexOf
?
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.
lastIndexOf
or indexOf
?
{
url: "/more/complex/path.js",
outputPath: "/a",
publicPath: "/complex"
}
Is it expected to be false
?
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 lastIndexOf
, see this question.
Yes, the example you've given is expected to be false
, because publicPath
is not in the url. Would be nice if you add your example to the tests.
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.
Ah, just saw your changes with indexOf
, that's also ok.
} | ||
|
||
if(!urlObject.hostname && localPrefix.hostname && | ||
!new RegExp('^' + localPrefix.path).test(url)) { |
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.
Same with the RegExp
as above.
One more thing, could you add a testcase that failed before, but is fixed now? Otherwise someone could refactor it again and stop your usecase from working ;). |
Thank you! |
This fix issue dvdzkwsk/react-redux-starter-kit#666 and as well Express 4 that req.path is undefined in some situation.