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.

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

feat: Depreciate toBeInTheDOM with replacement #40

Merged
merged 16 commits into from
Jul 18, 2018
Merged
Changes from 6 commits
Commits
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
4 changes: 4 additions & 0 deletions extend-expect.d.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
declare namespace jest {
interface Matchers<R> {
/**
* @deprecated
*/
toBeInTheDOM(container?: HTMLElement | SVGElement): R
toBeInTheDocument(): R
toBeVisible(): R
toBeEmpty(): R
toBeDisabled(): R
20 changes: 20 additions & 0 deletions src/__tests__/to-be-in-the-document.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
test('.toBeInTheDocument', () => {
document.body.innerHTML = `
<span data-testid="html-element"><span>Html Element</span></span>
<svg data-testid="svg-element"></svg>`

const htmlElement = document.querySelector('[data-testid="html-element"]')
const svgElement = document.querySelector('[data-testid="html-element"]')
const detachedElement = document.createElement('div')
const fakeElement = {thisIsNot: 'an html element'}

expect(htmlElement).toBeInTheDocument()
expect(svgElement).toBeInTheDocument()
expect(detachedElement).not.toBeInTheDocument()

// negative test cases wrapped in throwError assertions for coverage.
expect(() => expect(htmlElement).not.toBeInTheDocument()).toThrowError()
expect(() => expect(svgElement).not.toBeInTheDocument()).toThrowError()
expect(() => expect(detachedElement).toBeInTheDocument()).toThrowError()
expect(() => expect(fakeElement).toBeInTheDocument()).toThrowError()
})
5 changes: 5 additions & 0 deletions src/__tests__/to-be-in-the-dom.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import {render} from './helpers/test-utils'

test('.toBeInTheDOM', () => {
// @deprecated intentionally hiding warnings for test clarity
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})

const {queryByTestId} = render(`
<span data-testid="count-container">
<span data-testid="count-value"></span>
@@ -51,4 +54,6 @@ test('.toBeInTheDOM', () => {
expect(() => {
expect(valueElement).toBeInTheDOM(fakeElement)
}).toThrowError()

spy.mockRestore()
})
15 changes: 15 additions & 0 deletions src/__tests__/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import {deprecate, getDepreciationMessage} from '../utils'

test('deprecate', () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})

const messageWithReplacement = getDepreciationMessage('test', 'test')
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I commented below, I think we should type out the full expected message here, rather than rebuilding it again with the function.

deprecate('test', 'test')
expect(spy).toHaveBeenCalledWith(messageWithReplacement)

const messageWithoutReplacement = getDepreciationMessage('test')
deprecate('test')
expect(spy).toHaveBeenCalledWith(messageWithoutReplacement)

spy.mockRestore()
})
2 changes: 2 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {toBeInTheDOM} from './to-be-in-the-dom'
import {toBeInTheDocument} from './to-be-in-the-document'
import {toBeEmpty} from './to-be-empty'
import {toContainElement} from './to-contain-element'
import {toHaveTextContent} from './to-have-text-content'
@@ -11,6 +12,7 @@ import {toBeDisabled} from './to-be-disabled'

export {
toBeInTheDOM,
toBeInTheDocument,
toBeEmpty,
toContainElement,
toHaveTextContent,
24 changes: 24 additions & 0 deletions src/to-be-in-the-document.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import {matcherHint, printReceived} from 'jest-matcher-utils'
import {checkHtmlElement} from './utils'

export function toBeInTheDocument(element) {
checkHtmlElement(element, toBeInTheDocument, this)

return {
pass: document.documentElement.contains(element),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we check whether document and document.documentElement exist? I know jsdom will have them, but if I accidentally used this matcher in the node environment, it would be nice to get a descriptive error message rather than "cannot read property 'documentElement' of undefined".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hrm, sounds good, agreed on developer experience. It would be nice to get a clear error. Does the following sound good @jgoz?

document is undefined on global but is required for the toBeInTheDocument matcher.

documentElement is undefined on document but is required for the toBeInTheDocument matcher.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 the first message should be good enough for both cases.

message: () => {
return [
matcherHint(
`${this.isNot ? '.not' : ''}.toBeInTheDocument`,
'element',
'',
),
'',
'Received:',
` ${printReceived(
element.hasChildNodes() ? element.cloneNode(false) : element,
)}`,
].join('\n')
},
}
}
7 changes: 6 additions & 1 deletion src/to-be-in-the-dom.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import {matcherHint, printReceived} from 'jest-matcher-utils'
import {checkHtmlElement} from './utils'
import {checkHtmlElement, deprecate} from './utils'

export function toBeInTheDOM(element, container) {
deprecate(
'toBeInTheDOM',
'Please use toBeInTheDocument for searching the entire document and toContainElement for searching a specific container.',
)

if (element) {
checkHtmlElement(element, toBeInTheDOM, this)
}
27 changes: 26 additions & 1 deletion src/utils.js
Original file line number Diff line number Diff line change
@@ -66,4 +66,29 @@ function matches(textToMatch, node, matcher) {
}
}

export {checkHtmlElement, getMessage, matches}
function deprecate(name, replacementText) {
const message = getDepreciationMessage(name, replacementText)

// Notify user that they are using deprecated functionality.
// eslint-disable-next-line no-console
console.warn(message)
}

function getDepreciationMessage(name, replacementText) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be exposed as a separate function at all? I know it's used in the tests, but I would rather see the full message typed out in the tests than use this function in both spots. Otherwise we never really test the output of this function.

My preference would be to remove this function and inline the message creation in deprecate:

function deprecate(name, replacementText) {
  // Notify user that they are using deprecated functionality.
  // eslint-disable-next-line no-console
  console.warn(
    `Warning: ${name} has been deprecated and will be removed in future updates.`,
     replacementText
  )
}

Copy link
Collaborator Author

@smacpherson64 smacpherson64 Jul 14, 2018

Choose a reason for hiding this comment

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

Sounds good, I will aim to remove this and update the check to use the text.

return [
'Warning:',
name,
'has been deprecated and will be removed in future updates.',
replacementText,
]
.join(' ')
.trim()
}

export {
checkHtmlElement,
getMessage,
matches,
deprecate,
getDepreciationMessage,
}