fix: respect non-POJO objects in setValues #4289
Conversation
VladStepanov
force-pushed
the
fix/setValues-non-POJO
branch
from
9efddb4
to
0e41ff9
Compare
VladStepanov
force-pushed
the
fix/setValues-non-POJO
branch
from
0e41ff9
to
1b11751
Compare
There was a problem hiding this comment.
Very good stuff, I have a couple of nitpicks but otherwise looks good.
It would be nice if we could cover the uncovered lines in tests.
packages/shared/utils.ts
Outdated
|
|
||
| export function isPlainObject(value: any) { | ||
| // eslint-disable-next-line eqeqeq | ||
| if (!isObjectLike(value) || getTag(value) != '[object Object]') { |
There was a problem hiding this comment.
Any specific reason why we use != instead of !==. I would prefer we use !== instead of silencing the eslint rule.
There was a problem hiding this comment.
Maybe to install lodash and simply reuse _.isPlainObject https://lodash.com/docs/4.17.15#isPlainObject?
Because otherwise we will have to support and cover with tests in our codebase what is already covered in lodash
There was a problem hiding this comment.
I usually don't like including libs inside vee-validate due to bundle size. If it isn't far off (using Lodash.isPlainObject vs building our own) bundle-size wise (you can test it with pnpm build), Then happy to include it.
There was a problem hiding this comment.
Another option is to use deepmerge which is around 723B only and avoid having our own merge logic. But let's try out lodash-es first and see if it doesn't add much to the bundle size.
There was a problem hiding this comment.
I've tested bundle size and have following result:
Main branch size:
This is a base size
Output File: vee-validate.esm.js | Size: 123.59 kB | Gzip: 25.51 kB
Output File: vee-validate.js | Size: 123.07 kB | Gzip: 23.16 kB
Output File: vee-validate.min.js | Size: 40.04 kB | Gzip: 11.96 kBCase 1. Without lodash deps, only copy-paste sources
Output File: vee-validate.esm.js | Size: 124.29 kB | Gzip: 25.65 kB
Output File: vee-validate.js | Size: 123.87 kB | Gzip: 23.3 kB
Output File: vee-validate.min.js | Size: 40.4 kB | Gzip: 12.04 kBCase 2. With lodash deps
Output File: vee-validate.esm.js | Size: 129.37 kB | Gzip: 26.97 kB
Output File: vee-validate.js | Size: 119.8 kB | Gzip: 24.25 kB
Output File: vee-validate.min.js | Size: 41.21 kB | Gzip: 12.25 kBCase 3. deepmerge + is-plain-object
By default, deepmerge doesn't have isPlainObject guard, so they recommend install additional deps https://github.com/TehShrike/deepmerge#ismergeableobject
I suppose it's the worst one
Output File: vee-validate.esm.js | Size: 128.54 kB | Gzip: 26.9 kB
Output File: vee-validate.js | Size: 118.93 kB | Gzip: 24.19 kB
Output File: vee-validate.min.js | Size: 41.88 kB | Gzip: 12.52 kBWhat case may i implement?
There was a problem hiding this comment.
And one more case with lodash-es
Output File: vee-validate.esm.js | Size: 128.95 kB | Gzip: 26.89 kB
Output File: vee-validate.js | Size: 122.63 kB | Gzip: 24.28 kB
Output File: vee-validate.min.js | Size: 41.05 kB | Gzip: 12.24 kBThere was a problem hiding this comment.
Let's go with case 1, smallest is always the best. Thank you and I appreciate your patience with this PR.
There was a problem hiding this comment.
Implemented and added some tests for coverage ✅
packages/shared/utils.ts
Outdated
| export function getTag(value: any) { | ||
| if (value == null) { | ||
| return value === undefined ? '[object Undefined]' : '[object Null]'; | ||
| } | ||
| return Object.prototype.toString.call(value); | ||
| } | ||
|
|
||
| export function isPlainObject(value: any) { |
There was a problem hiding this comment.
Let's add a link for where this is pulled from, like lodash's src code link. Just to be a reference.
|
Amazing work, thank you a lot for reporting and owning the fix for this issue. I will tag with the next patch this evening or tomorrow. |
🔎 Overview
Fixes #4287
🤓 Code snippets/examples (if applicable)
✔ Issues affected
closes #4287