Warn when component option should be an object, but is not (#5605) #5642
Conversation
src/core/instance/state.js
Outdated
| @@ -148,8 +148,14 @@ function getData (data: Function, vm: Component): any { | |||
| 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')
| @@ -24,4 +24,11 @@ describe('Options methods', () => { | |||
| }) | |||
| 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 🙂
src/core/instance/state.js
Outdated
| @@ -60,6 +60,17 @@ const isReservedProp = { | |||
| 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
src/core/instance/state.js
Outdated
| 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')
src/core/instance/state.js
Outdated
| @@ -60,6 +60,17 @@ const isReservedProp = { | |||
| slot: 1 | |||
| } | |||
|
|
|||
| 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
src/core/instance/state.js
Outdated
| @@ -60,6 +60,17 @@ const isReservedProp = { | |||
| 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
src/core/instance/state.js
Outdated
| 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
#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: