feat(waitForElement): a wrapper around MutationObserver #10

sompylasar · 2018-04-08T10:52:52Z

Add waitForElement implementation, tests, docs
Add mutationobserver-shim to dependencies to provide
MutationObserver in jest tests where jsdom has no built-in support
for it: add support for MutationObserver jsdom/jsdom#639
Update @sompylasar contributions

Add `mutationobserver-shim` to `devDependencies` to provide `MutationObserver` in jest tests where `jsdom` has no built-in support for it: jsdom/jsdom#639

kentcdodds

This is super!

So I guess we're not providing them with the mutation observer polyfill ourselves? They have to install that? I'd rather polyfill it for people I think.

Thanks for this!

kentcdodds · 2018-04-08T12:58:48Z

README.md

+// ...
+// wait until the callback does not throw an error and returns a truthy value. In this case, that means
+// it'll wait until we can get a form control with a label that matches "username"
+// the difference from `wait` is that it reacts to DOM changes in the container


Would this make it more clear?

// the difference from `wait` is that rather than running your callback on // an interval, it's run as soon as there are DOM changes in the container // and returns the value returned by the callback

kentcdodds · 2018-04-08T12:59:25Z

README.md

+// it'll wait until we can get a form control with a label that matches "username"
+// the difference from `wait` is that it reacts to DOM changes in the container
+// and returns the value returned by the callback
+const element = await waitForElements(() => getByLabelText(container, 'username'))


Let's make this more clear by changing element to usernameElement

kentcdodds · 2018-04-08T13:00:36Z

README.md

+Defined as:
+
+```typescript
+function waitForElements<T>(


Normally I think people will be looking for a single element. Should this be called waitForElement?

In addition, we should show an example (in the FAQ?) of getting an array of elements:

const elements = waitForElement(() => [ getByText('username'), getByText('password') ])

kentcdodds · 2018-04-08T13:01:24Z

README.md

+// ...
+```
+
+Using `MutationObserver` is more efficient than polling the DOM at regular intervals with `wait`.


Let's add a link to the MDN docs for this so folks know that it's actually a browser feature. Most people have probably never heard of it. I only heard of it a few weeks ago when building the cypress command waiter.

Note that I added the link in the above paragraph. But I'll add here as well.

kentcdodds · 2018-04-08T13:02:18Z

README.md

+Using `MutationObserver` is more efficient than polling the DOM at regular intervals with `wait`.
+
+The default `callback` is a no-op function (used like `await waitForElements()`). This can
+be helpful if you only need to wait for the next DOM change (see `mutationObserverOptions` to learn which changes are detected).


Add a link to mutationObserverOptions :)

kentcdodds · 2018-04-08T13:02:58Z

README.md

+
+The default `mutationObserverOptions` is `{subtree: true, childList: true}` which will detect
+additions and removals of child elements (including text nodes) in the `container` and any of its descendants.
+It won't detect attribute changes unless you add `attributes: true` to the options.


Perfect 👍

kentcdodds · 2018-04-08T13:03:09Z

package.json

@@ -39,7 +39,8 @@
  },
  "devDependencies": {
    "jest-in-case": "^1.0.2",
-    "kcd-scripts": "^0.36.1"
+    "kcd-scripts": "^0.36.1",
+    "mutationobserver-shim": "^0.3.2"


This should be a regular dependency.

kentcdodds · 2018-04-08T13:04:50Z

src/waitForElements.js

+) {
+  return new Promise((resolve, reject) => {
+    // Disabling eslint prefer-const below: either prefer-const or no-use-before-define triggers.
+    let lastError, observer, timer // eslint-disable-line prefer-const


Really odd that both of these rules trigger 🤔 I don't see how this code is in violation. An ESLint bug?

If I have it as const, I have to initialize it at definition, i.e. at this line:

timer = setTimeout(onTimeout, timeout)

but then:

function onDone(error, result) { clearTimeout(timer)

is a usage before definition of timer

kentcdodds · 2018-04-08T13:06:07Z

src/waitForElements.js

+    timer = setTimeout(onTimeout, timeout)
+    observer = new MutationObserver(onMutation)
+    observer.observe(container, mutationObserverOptions)
+  })


Could we run onMutation above this line? That way if there's no reason to wait for a mutation to proceed we don't have to wait for one.

sompylasar · 2018-04-08T22:14:28Z

So I guess we're not providing them with the mutation observer polyfill ourselves? They have to install that? I'd rather polyfill it for people I think.

We could bundle it I guess, yes.

- rename `waitForElements` to `waitForElement` - move `mutationobserver-shim` to dependencies, import from `waitForElement` to provide the polyfill to the users of `dom-testing-library` - fix `kcd-scripts` version to match `master` branch - add synchronous `callback` call to detect an element if it's already present before any DOM mutation happens - add/change tests about synchronous `callback` call - tweak variable names in docs examples - add docs about the default `container` option value - add docs example about querying multiple elements - add docs about the `mutationobserver-shim` polyfill - add docs link and anchor to `mutationObserverOptions` - add docs link to MDN from the second mention of `MutationObserver`

sompylasar · 2018-04-08T22:51:31Z

@kentcdodds All requested changes addressed here: 1f46848

kentcdodds

This is looking awesome! Actually, I'm thinking we should recommend this over wait. Do you want to update other examples in the tests and README examples where it makes sense? Thanks a bunch!

kentcdodds · 2018-04-08T23:12:25Z

README.md

+The default `callback` is a no-op function (used like `await waitForElement()`). This can
+be helpful if you only need to wait for the next DOM change (see [`mutationObserverOptions`](#mutationobserveroptions) to learn which changes are detected).
+
+The default `container` is the global `document`. Make sure the elements you wait for will be attached to it, or set a different `container`.


Could you add a simple example of this. It's probably the most common use case

Or maybe just make the example above use it like that.

Should wait for the next DOM change, as advertised in the docs. The default value is `undefined` so that the `options` object can be used while still keeping the default callback: ``` waitForElement(undefined, {attributes: true}) ```

- use `container` in the examples as this is a more popular use case than the default value of global `document` - use full sentences with capital first letter and period in the example comments

sompylasar · 2018-04-09T01:16:03Z

Turned out the no-argument waitForElement() stopped working as advertised after we've added the synchronous onMutation() call. Fixed it here: 88bab87

Added container option to the examples here: b7158cf

sompylasar · 2018-04-09T01:22:10Z

Actually, I'm thinking we should recommend this over wait. Do you want to update other examples in the tests and README examples where it makes sense?

I'm not sure how you want me to do this, so I'd rather leave this change for you to make later if you don't mind.

I can reorder the wait and waitForElement sections in the README, but then it needs rewriting a bunch of texts that compare waitForElement to wait. From this point this starts to look like a bikeshedding, I wanted to incorporate it into cypress-testing-library this weekend, but this PR is already taking longer than expected.

kentcdodds

Looking great! A few items of clarification.

Also, it just occurred to me that this might be a really useful library on its own. 🤔

kentcdodds · 2018-04-09T01:22:52Z

src/index.js

@@ -6,4 +6,5 @@ export {queries}

 export * from './queries'
 export * from './wait'
+export * from './waitForElement'


This is a bit of a nit-pick and I really should have an eslint rule for this, but due to issues with file casing + different operating systems + git, I prefer filenames to always use kebab-case rather than camelCase. Could you update the files to use kebab-case please?

Okay, but tell this to React team. It's for the sake of searchability that the file is literally named the same as the thing it contains and exports. Better have a lint rule that ensures the names strictly match case with the filesystem (and there is such, I think in the eslint-plugin-import).

I have told them and they rejected it. But I've had too many people bitten by file casing issues across operating systems to do anything else.

Okay we have different experiences (mine is about large codebases where searchability is key, akin to Facebook). I'll update when I get back to it.

If searchability is key then you say: "Oh, here's a thing called waitForElement in the code, so I'll turn that to kebab-case and search for wait-for-element." Believe me, I've felt these pains before as well. We just have different solutions.

In addition, it's why I'm starting to avoid default exports now because the greppability of a named export is better.

I just select the thing, cmd+shift+f, no need to convert. Especially when doing large refactorings. Default exports I agree can become messy if coders don't follow to name them the same at import site, it's a matter of discipline which is not enforced by the program structure.

👉 1252d08

kentcdodds · 2018-04-09T01:24:19Z