-
-
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
c22dad0
fix(client): stricter reg exp to redirect sockjs client path
knagaitsev 9d44831
fix(client): check resource context for normal module replacement
knagaitsev 4c68543
fix(client): remove dev server string from context path
knagaitsev ecf61e3
fix(client): remove console logs
knagaitsev eda4074
fix(client): fixed resource context match to use cwd
knagaitsev c8d90ce
Merge branch 'master' into strict-reg-exp-fix
hiroppy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
'use strict'; | ||
|
||
const testServer = require('../helpers/test-server'); | ||
const config = require('../fixtures/client-config/webpack.config'); | ||
const runBrowser = require('../helpers/run-browser'); | ||
const port = require('../ports-map').Iframe; | ||
|
||
// iframe mode should be tested while still supported, because | ||
// its sources differ from those of inline mode, which can cause unexpected | ||
// breaking changes: https://github.com/webpack/webpack-dev-server/issues/2006 | ||
describe('Client iframe console.log', () => { | ||
const baseOptions = { | ||
port, | ||
host: '0.0.0.0', | ||
}; | ||
const cases = [ | ||
{ | ||
title: 'hot disabled', | ||
options: { | ||
hot: false, | ||
}, | ||
}, | ||
{ | ||
title: 'hot enabled', | ||
options: { | ||
hot: true, | ||
}, | ||
}, | ||
{ | ||
title: 'liveReload disabled', | ||
options: { | ||
liveReload: false, | ||
}, | ||
}, | ||
{ | ||
title: 'liveReload enabled', | ||
options: { | ||
liveReload: true, | ||
}, | ||
}, | ||
{ | ||
title: 'clientLogLevel is silent', | ||
options: { | ||
clientLogLevel: 'silent', | ||
}, | ||
}, | ||
]; | ||
|
||
for (const { title, options } of cases) { | ||
it(title, () => { | ||
const res = []; | ||
const testOptions = Object.assign({}, baseOptions, options); | ||
|
||
// TODO: use async/await when Node.js v6 support is dropped | ||
return Promise.resolve() | ||
.then(() => { | ||
return new Promise((resolve) => { | ||
testServer.startAwaitingCompilation(config, testOptions, resolve); | ||
}); | ||
}) | ||
.then(runBrowser) | ||
.then(({ page, browser }) => { | ||
return new Promise((resolve) => { | ||
page.goto(`http://localhost:${port}/webpack-dev-server/main`); | ||
page.on('console', ({ _text }) => { | ||
res.push(_text); | ||
}); | ||
setTimeout(() => { | ||
browser.close().then(() => { | ||
expect(res).toMatchSnapshot(); | ||
resolve(); | ||
}); | ||
}, 3000); | ||
}); | ||
}) | ||
.then(() => { | ||
return new Promise((resolve) => { | ||
testServer.close(resolve); | ||
}); | ||
}); | ||
}); | ||
} | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`Client iframe console.log clientLogLevel is silent 1`] = ` | ||
Array [ | ||
"Hey.", | ||
] | ||
`; | ||
|
||
exports[`Client iframe console.log hot disabled 1`] = ` | ||
Array [ | ||
"Hey.", | ||
"[WDS] Live Reloading enabled.", | ||
] | ||
`; | ||
|
||
exports[`Client iframe console.log hot enabled 1`] = ` | ||
Array [ | ||
"[HMR] Waiting for update signal from WDS...", | ||
"Hey.", | ||
"[WDS] Hot Module Replacement enabled.", | ||
"[WDS] Live Reloading enabled.", | ||
] | ||
`; | ||
|
||
exports[`Client iframe console.log liveReload disabled 1`] = ` | ||
Array [ | ||
"Hey.", | ||
] | ||
`; | ||
|
||
exports[`Client iframe console.log liveReload enabled 1`] = ` | ||
Array [ | ||
"Hey.", | ||
] | ||
`; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 codeThere 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 awebpack-dev-server
directory if someone clones it into a different named directory. If we want to be very strict about it, we could match againstpath.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