updates example - catches the case whereby assets are an object #311
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #311 +/- ##
=======================================
Coverage 96.81% 96.81%
=======================================
Files 7 7
Lines 251 251
=======================================
Hits 243 243
Misses 8 8Continue to review full report at Codecov.
|
README.md
Outdated
| @@ -318,7 +318,7 @@ const middleware = require('webpack-dev-middleware'); | |||
|
|
|||
| // This function makes server rendering of asset references consistent with different webpack chunk/entry configurations | |||
| function normalizeAssets(assets) { | |||
| if (typeof assets === 'object') { | |||
| if (Object.prototype.toString.call(assets) === '[object Object]') { | |||
There was a problem hiding this comment.
this is not a good change, as there are many different cases which qualify an entity as an object. you should recommend use of a module like is-plain-obj instead
There was a problem hiding this comment.
@shellscape Yes I'm just realising this now, will change
There was a problem hiding this comment.
isObject from lodash is okay, but it will make a big dependency. When checking object type i usually go for typeof assets === Object or assets instanceof Object
There was a problem hiding this comment.
@ev1stensberg but the problem there is
There was a problem hiding this comment.
either of those checks would catch arrays
There was a problem hiding this comment.
yeah just go with a package to make this example simple. we don't need to bikeshed over that, we just want something simple for users to consume in the docs. if they prefer another method, I'm sure they'll use it.
correct typecheck in SSR example
uses helper for isObject
|
@michael-ciniawsky same problem with tests here? |
|
Yes, it's a flake (we need to investigate separately) :) The docs update likely won't have had an effect here :D 😛 |
What kind of change does this PR introduce?
Change to the SSR example in the README
Did you add tests for your changes?
tests not required
Summary
when following this example for SSR I added a check for the case in
normalizeAssetswhereby an object will be converted to an array. This makes sense for the purpose when your webpack entry is an object. Then you will get an array of paths.Does this PR introduce a breaking change?
no
Other information
I'm not an expert and maybe there is an edge case where this method is unsuitable, but it works for my use case and would maybe help someone else too