Skip to content

feat: throw error if the read-only property is tried to change #749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 24, 2018
Merged

feat: throw error if the read-only property is tried to change #749

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5950f50
feat: change WrapperArray.wrappers and WrapperArray.length to read-only
38elements Jun 23, 2018
974be4f
throw error
38elements Jun 23, 2018
994018f
fix error message
38elements Jun 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions docs/api/wrapper-array/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ A `WrapperArray` is an object that contains an array of [`Wrappers`](../wrapper/

## Properties

### `wrappers`
### `wrappers`

`array`: the `Wrappers` contained in the `WrapperArray`
`array` (read-only): the `Wrappers` contained in the `WrapperArray`

### `length`
### `length`

`number`: the number of `Wrappers` contained in the `WrapperArray`
`number` (read-only): the number of `Wrappers` contained in the `WrapperArray`

## Methods

Expand Down
4 changes: 2 additions & 2 deletions docs/api/wrapper/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ A `Wrapper` is an object that contains a mounted component or vnode and methods

`HTMLElement` (read-only): the root DOM node of the wrapper

### `options`
### `options`

#### `options.attachedToDocument`

`Boolean` (read-only): True if `attachedToDocument` in mounting options was `true`

#### `options.sync`
#### `options.sync`

`Boolean` (read-only): True if `sync` in mounting options was not `false`

Expand Down
17 changes: 13 additions & 4 deletions packages/test-utils/src/wrapper-array.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,21 @@ import type VueWrapper from './vue-wrapper'
import { throwError, warn } from 'shared/util'

export default class WrapperArray implements BaseWrapper {
wrappers: Array<Wrapper | VueWrapper>;
length: number;
+wrappers: Array<Wrapper | VueWrapper>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool I didn't know you could specify read only with flow 👍

+length: number;

constructor (wrappers: Array<Wrapper | VueWrapper>) {
this.wrappers = wrappers || []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrappers is always an Array object.

this.length = this.wrappers.length
const length = wrappers.length
// $FlowIgnore
Object.defineProperty(this, 'wrappers', {
get: () => wrappers,
set: () => {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should throw an error to tell the users it's read only.

I realize this isn't part of this PR, but I think we should do the same for vnode, and element.

Copy link
Contributor Author

@38elements 38elements Jun 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing out.
I will change them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vnode is undocmumented.
Should vnode is added to document?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. No I don't think it needs to be documented

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reply.
I think so too.

})
// $FlowIgnore
Object.defineProperty(this, 'length', {
get: () => length,
set: () => {}
})
}

at (index: number): Wrapper | VueWrapper {
Expand Down
2 changes: 1 addition & 1 deletion packages/test-utils/src/wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,7 @@ export default class Wrapper implements BaseWrapper {
*/
destroy () {
if (!this.isVueInstance()) {
throwError(`wrapper.destroy() can only be called on a Vue ` + `instance`)
throwError(`wrapper.destroy() can only be called on a Vue instance`)
}

if (this.element.parentNode) {
Expand Down
12 changes: 3 additions & 9 deletions test/specs/wrapper-array.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@ describeWithShallowAndMount('WrapperArray', mountingMethod => {
const wrapper = mountingMethod(compiled)
const wrapperArray = wrapper.findAll('p')
expect(wrapperArray.constructor.name).to.equal('WrapperArray')
if (wrappers) {
wrapperArray.wrappers = wrappers
wrapperArray.length = wrappers.length
}
return wrapperArray
return wrappers ? new wrapperArray.constructor(wrappers) : wrapperArray
}

it('returns class with length equal to length of wrappers passed in constructor', () => {
Expand Down Expand Up @@ -67,8 +63,7 @@ describeWithShallowAndMount('WrapperArray', mountingMethod => {
if (method === 'at') {
return
}
const wrapperArray = getWrapperArray()
wrapperArray.wrappers = []
const wrapperArray = getWrapperArray([])
const message = `[vue-test-utils]: ${method} cannot be called on 0 items`
expect(() => wrapperArray[method]())
.to.throw()
Expand Down Expand Up @@ -99,8 +94,7 @@ describeWithShallowAndMount('WrapperArray', mountingMethod => {
) {
return
}
const wrapperArray = getWrapperArray()
wrapperArray.wrappers = [1, 2, 3]
const wrapperArray = getWrapperArray([1, 2, 3])
const message = `[vue-test-utils]: ${method} must be called on a single wrapper, use at(i) to access a wrapper`
expect(() => wrapperArray[method]())
.to.throw()
Expand Down