refactor(client): migrate client to built-in logger #2631
Conversation
Codecov Report
@@ Coverage Diff @@
## v4 #2631 +/- ##
==========================================
+ Coverage 92.35% 92.46% +0.10%
==========================================
Files 34 34
Lines 1321 1300 -21
Branches 373 365 -8
==========================================
- Hits 1220 1202 -18
+ Misses 96 93 -3
Partials 5 5
Continue to review full report at Codecov.
|
| @@ -1,5 +1,11 @@ | |||
| 'use strict'; | |||
|
|
|||
| // webpack@5 doesn't inject node polyfill automatically | |||
| window.global = window.global || window; | |||
There was a problem hiding this comment.
this was needed to make the other polyfills work. Is this way to handle this okay, or is it a problem that this is ignoring webpack's node option https://webpack.js.org/configuration/node/ ?
Any suggestions?
There was a problem hiding this comment.
Hm, why we need process and util?
There was a problem hiding this comment.
Hm, why we need process and util?
@evilebottnawi webpack5 tests were not passing, this made them work. Not sure if there is a better solution. @hiroppy Originally put that in here:
b6af4e9#diff-bd664d218d585ff7bd6ceced46bd877aR3
There was a problem hiding this comment.
We can use alias here when we will switch on webpack@5, we really need full process/browser and util/util packages? Maybe we can copy post some function to avoid using full packages?
There was a problem hiding this comment.
do you mean resolve.alias? https://webpack.js.org/configuration/resolve/#resolvealias
lib/options.json
Outdated
| "none", | ||
| "warning" | ||
| ] | ||
| "enum": ["none", "error", "warn", "info", "log", "verbose"] |
There was a problem hiding this comment.
Let's move it to clientOptions, i.e.:
clientOptions: {
logging: {
level: 'info',
// Let's avoid implement debug in this PR, just for future, schema should not contain debug too
debug: [
'MyPlugin',
/MyPlugin/,
(name) => name.contains('MyPlugin')
]
}
}
Like here https://webpack.js.org/configuration/other-options/#infrastructurelogging
There was a problem hiding this comment.
@evilebottnawi Should we rename the client-log-level option then? Maybe client-logging? I think just logging might be confusing.
There was a problem hiding this comment.
@Loonride webpack has infrastructureLogging, so better to keep this same - https://webpack.js.org/configuration/other-options/
There was a problem hiding this comment.
I think we should move all client options to clientOptions, maybe we should rename clientOptions to client, less characters in the name, what do you think?
There was a problem hiding this comment.
Yes, I moved logging into clientOptions. I agree that it should be renamed to client, but that is an easy fix, I will do it later
client-src/default/index.js
Outdated
| @@ -55,17 +61,20 @@ const onSocketMessage = { | |||
| status.currentHash = hash; | |||
| }, | |||
| 'still-ok': function stillOk() { | |||
| log.info('[WDS] Nothing changed.'); | |||
| log.info('Nothing changed.'); | |||
| if (options.useWarningOverlay || options.useErrorOverlay) { | |||
| overlay.clear(); | |||
| } | |||
| sendMessage('StillOk'); | |||
| }, | |||
| 'log-level': function logLevel(level) { | |||
There was a problem hiding this comment.
Let't rename it to logging
| @@ -1,5 +1,11 @@ | |||
| 'use strict'; | |||
|
|
|||
| // webpack@5 doesn't inject node polyfill automatically | |||
| window.global = window.global || window; | |||
There was a problem hiding this comment.
Hm, why we need process and util?
|
/cc @Loonride |
knagaitsev
force-pushed
the
refactor/client-builtin-logger
branch
from
7ca1907 to
eaa9d15
Compare
| // webpack@5 doesn't inject node polyfill automatically | ||
| window.global = window.global || window; | ||
| window.process = window.process || {}; | ||
| window.process.env = window.process.env || {}; |
There was a problem hiding this comment.
@evilebottnawi I substituted process/browser out for this, I think it is all that is needed. I think we still need the util/util package as a dependency, since something in tapable needs it.
knagaitsev
force-pushed
the
refactor/client-builtin-logger
branch
from
863fef8 to
0afc8c6
Compare
|
Proper handling of babel for the client should be merged before this: #2652 |
For Bugs and Features; did you add new tests?
Motivation / Use-Case
Migrating to built-in logger in client
Followed slightly different approach for the actual log util than #2563
Breaking Changes
Yes, new client log levels
@hiroppy Do we need to worry about this for webpack@5 b6af4e9#diff-bd664d218d585ff7bd6ceced46bd877aR3
Additional Info