-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
refactor(client): separate into modules #1948
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1948 +/- ##
=========================================
+ Coverage 91.54% 91.8% +0.26%
=========================================
Files 19 22 +3
Lines 840 879 +39
Branches 263 276 +13
=========================================
+ Hits 769 807 +38
- Misses 68 69 +1
Partials 3 3
Continue to review full report at Codecov.
|
960df69
to
2a1166e
Compare
client-src/default/index.js
Outdated
if (typeof window !== 'undefined') { | ||
const qs = window.location.search.toLowerCase(); | ||
hotReload = qs.indexOf('hotreload=false') === -1; | ||
options.hotReload = !qs.includes('hotreload=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.
Can't be used, includes
is not supported old browsers, we can change this only in major release
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 we need setup polyfill for this
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.
ok, I'll add babel-polyfill
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.
@hiroppy let's do this in other PR, using babel-polyfill
directly can break many apps, because only one babel-polyfill
can be defined, we should using core-js
directly
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.
@hiroppy let's leave this as is in this PR and change this in other PR (where we include polyfills)
throw new Error('[WDS] Failed to get current script source.'); | ||
} | ||
const sendMessage = require('./utils/sendMessage'); | ||
const reloadApp = require('./utils/reloadApp'); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@@ -0,0 +1,49 @@ | |||
'use strict'; |
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.
Let's rename test directory to client
not client-src
. src
postfix used because we build bundle
client-src/default/index.js
Outdated
|
||
// check ipv4 and ipv6 `all hostname` | ||
if (hostname === '0.0.0.0' || hostname === '::') { | ||
// why do we need this check? | ||
// hostname n/a for file protocol (example, when using electron, ionic) | ||
// see: https://github.com/webpack/webpack-dev-server/pull/384 | ||
// eslint-disable-next-line no-bitwise | ||
if (self.location.hostname && !!~self.location.protocol.indexOf('http')) { | ||
if (self.location.hostname && self.location.protocol.includes('http')) { |
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.
Also don't use includes
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.
Let's leave as is in this PR as i said above
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, some notes
2a1166e
to
ee3bd45
Compare
/cc @evilebottnawi PTAL |
ee3bd45
to
fbaec41
Compare
updated snapshots |
Codecov Report
@@ Coverage Diff @@
## master #1948 +/- ##
=========================================
+ Coverage 91.54% 91.8% +0.26%
=========================================
Files 19 22 +3
Lines 840 879 +39
Branches 263 276 +13
=========================================
+ Hits 769 807 +38
- Misses 68 69 +1
Partials 3 3
Continue to review full report at Codecov.
|
For Bugs and Features; did you add new tests?
yep
Motivation / Use-Case
This is the first step to refactor client code.
Breaking Changes
no
Additional Info