Warn when component option should be an object, but is not (#5605)#5642
Warn when component option should be an object, but is not (#5605)#5642
Conversation
| const computedWatcherOptions = { lazy: true } | ||
|
|
||
| function initComputed (vm: Component, computed: Object) { | ||
| if (!isPlainObject(computed)) { |
There was a problem hiding this comment.
You should refactor this block into a function and add the check there too so it is stripped when bundling:
process.env.NODE_ENV !== 'production' && checkOptionType(vm, 'computed')
| expect(`method "hello" has an undefined value in the component definition`).toHaveBeenWarned() | ||
| }) | ||
|
|
||
| it('should warn non object', () => { |
There was a problem hiding this comment.
These tests can also be refactored into a function. It should also test it doesn't print the warning in the right scenario
There was a problem hiding this comment.
Should refactored function be a separate file in test/helpers/?
There was a problem hiding this comment.
yes, after all you need to include it in different test files 🙂
| slot: 1 | ||
| } | ||
|
|
||
| function parseObjectOption (vm: Component, option: Object, name: string) { |
There was a problem hiding this comment.
no need for option arg because you have name
| vm | ||
| ) | ||
| } | ||
| computed = parseObjectOption(vm, computed, 'computed') |
There was a problem hiding this comment.
This must be called only in non-production env (NODE_ENV !== 'production')
|
|
||
| function parseObjectOption (vm: Component, option: Object, name: string) { | ||
| if (!isPlainObject(option)) { | ||
| process.env.NODE_ENV !== 'production' && warn( |
There was a problem hiding this comment.
since this function is only called in dev mode, this check is unecessary
There was a problem hiding this comment.
it may be good to provide a fallback (empty object) if a wrong type is used in prod mode
There was a problem hiding this comment.
no, because it will never reach production code
There was a problem hiding this comment.
The check must be removed in production
| slot: 1 | ||
| } | ||
|
|
||
| function parseObjectOption (vm: Component, option: Object, name: string) { |
There was a problem hiding this comment.
the result is used in assignment, to call it a "check" may be misleading
There was a problem hiding this comment.
The assignment prevents runtime errors but the function is meant to do a check
| const option = vm.$options[name] | ||
| if (!isPlainObject(option)) { | ||
| warn( | ||
| `${name} should be an object.`, |
There was a problem hiding this comment.
The message could be a bit more explicit: component option "${name}" should be an object
3 participants
#5605
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
devbranch for v2.x (or to a previous version branch), not themasterbranchfix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information: