-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(client): stricter reg exp to redirect sockjs client path #2069
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2069 +/- ##
==========================================
+ Coverage 94.56% 94.65% +0.08%
==========================================
Files 32 32
Lines 1215 1215
Branches 340 340
==========================================
+ Hits 1149 1150 +1
+ Misses 64 63 -1
Partials 2 2
Continue to review full report at Codecov.
|
); | ||
}), | ||
new webpack.NormalModuleReplacementPlugin( | ||
/^\.\/clients\/SockJSClient$/, |
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.
We need search way how we can check what module was loaded from our client-src
- it is right way don't break other 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.
You mean to test that it is loading the correct module in the webpack compilation? Could we run this webpack config in a test, look at the webpack CLI output, and see if it contains the right path?
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.
Yes, but it can solved using NormalModuleReplacementPlugin
as you use, just need more checks (need check context contains 'webpack-dev-server/client-src' and rewrite only in this case)
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.
@evilebottnawi I did something like this, except instead I matched '/client-src/default'
, because it is possible the dev server is not in a webpack-dev-server
directory if someone clones it into a different named directory. If we want to be very strict about it, we could match against path.resolve
to get the exact context we want, but I'm afraid that could result in some problem.
Also, I included backwards slash (\\
) in my matching string because windows seems to use that for the context paths
67c9759
to
9721b30
Compare
9721b30
to
ecf61e3
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.
Just check if (resource.context.startsWith(process.cwd())) { }
, we need rewrite clients
only for webpack-dev-server
, for other package it should be not rewritting
Done. Good solution! |
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.
/cc @hiroppy
…#2069) * fix(client): stricter reg exp to redirect sockjs client path * fix(client): check resource context for normal module replacement * fix(client): remove dev server string from context path * fix(client): remove console logs * fix(client): fixed resource context match to use cwd
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
Fixes problem introduced by #2015 using a stricter reg exp.
I added an iframe e2e test because the issue came up from users using
iframe
mode: #2006Breaking Changes
None
Additional Info