refactor: refactor some code #614
Conversation
Codecov Report
@@ Coverage Diff @@
## master #614 +/- ##
==========================================
- Coverage 99.12% 99.11% -0.01%
==========================================
Files 8 8
Lines 228 227 -1
Branches 71 70 -1
==========================================
- Hits 226 225 -1
Misses 2 2
Continue to review full report at Codecov.
|
src/utils/setupWriteToDisk.js
Outdated
| @@ -1,17 +1,19 @@ | |||
| import fs from 'fs'; | |||
| import path from 'path'; | |||
|
|
|||
| import { pluginName } from '../constants'; | |||
There was a problem hiding this comment.
No need extra require for one string, keep it as is in source code
src/utils/setupHooks.js
Outdated
| context.compiler.hooks.done.tap('DevMiddleware', done); | ||
| context.compiler.hooks.watchRun.tap(pluginName, invalid); | ||
| context.compiler.hooks.invalid.tap(pluginName, invalid); | ||
| context.compiler.hooks.done.tap(pluginName, done); |
There was a problem hiding this comment.
No need extra require, we don't change these lines very often
src/utils/getFilenameFromUrl.js
Outdated
| const { options } = context; | ||
| const paths = getPaths(context.stats, options); | ||
| const memoizedParse = mem(parse); |
There was a problem hiding this comment.
Memorize is broken, no need create it every time on each call
src/utils/getFilenameFromUrl.js
Outdated
| @@ -5,7 +5,7 @@ import querystring from 'querystring'; | |||
| import mem from 'mem'; | |||
|
|
|||
| function getPaths(stats, options) { | |||
| const childStats = stats.stats ? stats.stats : [stats]; | |||
| const childStats = Array.isArray(stats.stats) ? stats.stats : [stats]; | |||
There was a problem hiding this comment.
Extra check, stats.stats exists only for mutli compiler mode
| ready(context, processRequest, req); | ||
| }); | ||
| res.send(content); | ||
| } |
There was a problem hiding this comment.
This is the most important part and any change here is critical, very difficult to read
There was a problem hiding this comment.
Maybe you can add comments before main steps? I think it was necessary to do this earlier.
There was a problem hiding this comment.
Could you give me a comment? So you mean move res.send(content); to prev position?
There was a problem hiding this comment.
No, just add comments something like:
// Check headers
// Search file in compilation
// Setup content-length for file
// Sending file
There was a problem hiding this comment.
If you have points which are hard to read for you, please tell me.
src/middleware.js
Outdated
| } | ||
| if (headers) { | ||
| for (const name in headers) { | ||
| if (Reflect.has(headers, name)) { |
There was a problem hiding this comment.
{}.hasOwnProperty.call(headers, name) is very poorly readable.
There was a problem hiding this comment.
Hm, looks Reflect has good supporting, so it is good to use it
There was a problem hiding this comment.
Actually, this code is unnecessary. I updated.
hiroppy
force-pushed
the
feature/refactor-code
branch
from
44a5ebf
to
b007060
Compare
|
azure-pipeline seems not good. It can't download Node.js image. |
|
@hiroppy Let's resolve the PR with migration on github actions |
|
@evilebottnawi This pr has already been done. Please review. |
|
CI is green. |
|
Thanks! |
This PR contains a:
Motivation / Use-Case
Improve readability.
Breaking Changes
no
Additional Info