Skip to content
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

Jump to bottom

Fix typing for hasProperty assertion #782

Merged
merged 1 commit into from
Aug 13, 2020
+45 −3
Merged

Fix typing for hasProperty assertion #782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ expression.
#### Parameters

- `name` **[string][121]**
- `value` **([string][121] \| [RegExp][132])**
- `value` **([RegExp][132] | any)**
- `message` **[string][121]?**

#### Examples
Expand Down
42 changes: 42 additions & 0 deletions lib/__tests__/has-property.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,48 @@ describe('assert.dom(...).hasProperty()', () => {
});
});

describe('boolean expected', () => {
test('succeeds for correct name and value', () => {
assert.dom('input').hasProperty('disabled', false);
assert.dom(document.querySelector('input')).hasProperty('disabled', false);

expect(assert.results).toEqual([
{
actual: 'Element input has property "disabled" with value false',
expected: 'Element input has property "disabled" with value false',
message: 'Element input has property "disabled" with value false',
result: true,
},
{
actual: 'Element input[type="password"] has property "disabled" with value false',
expected: 'Element input[type="password"] has property "disabled" with value false',
message: 'Element input[type="password"] has property "disabled" with value false',
result: true,
},
]);
});

test('fails for wrong value', () => {
assert.dom('input').hasProperty('disabled', true);
assert.dom(document.querySelector('input')).hasProperty('disabled', true);

expect(assert.results).toEqual([
{
actual: 'Element input has property "disabled" with value false',
expected: 'Element input has property "disabled" with value true',
message: 'Element input has property "disabled" with value true',
result: false,
},
{
actual: 'Element input[type="password"] has property "disabled" with value false',
expected: 'Element input[type="password"] has property "disabled" with value true',
message: 'Element input[type="password"] has property "disabled" with value true',
result: false,
},
]);
});
});

describe('regex expected', () => {
test('succeeds for matching name and value', () => {
assert.dom('input').hasProperty('type', /^pass/);
Expand Down
4 changes: 2 additions & 2 deletions lib/assertions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,15 +520,15 @@ export default class DOMAssertions {
* expression.
*
* @param {string} name
* @param {string|RegExp} value
* @param {RegExp|any} value
* @param {string?} message
*
* @example
* assert.dom('input.password-input').hasProperty('type', 'password');
*
* @see {@link #doesNotHaveProperty}
*/
hasProperty(name: string, value: string | RegExp, message?: string): DOMAssertions {
hasProperty(name: string, value: unknown, message?: string): DOMAssertions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you using unknown here instead of RegExp | any as specified above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RegExp | any doesn’t make any sense in typescript, it will still evaluate as any. And unknown is safer then any because it doesn’t imply any knowledge about type.

As for JSDoc annotation there’s no unknown type, so any is the closest alternative. And it’s still helpful to mention RegExp because it has special treatment (match instead of just equality check).

Copy link

Choose a reason for hiding this comment

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

@wuron is correct: types in TS are sets, and since any is already a superset of everything else, having T | any, for any type, is always exactly identical to just having any.

Copy link

Choose a reason for hiding this comment

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

Addendum: you can actually use unknown in JSDoc and most tools will just show "unknown" there, and anything TS-aware will actually correctly interpret it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can actually use unknown in JSDoc and most tools will just show "unknown" there, and anything TS-aware will actually correctly interpret it.

If that’s preferred I can set it to RegExp | unknown in JSDoc. But does it really change anything? JSDoc definitions are only used as documentation: to generate API.md file, or to hint types for non-TS environments. In both cases we want to communicate that this method accepts any value, hence any works just fine.

In TS definitions we shouldn’t use any, because it’s compatible with all types and allows us to do something like this without any warnings:

function hasProperty(param: any) {
  param.callRandomMethod();
  const a: boolean = param;
  const b: string = param;
}

Parameter with type unknown is much safer, and it still allows to pass any value.

let element = this.findTargetElement();
if (!element) return this;

Expand Down