Add stopOnDomReady option, remove .cancel(), add .stop()

[fregante]

Requires semver major because it changes the default behavior.

• Approval
• index.test-d.ts with null, why is it failing?
• Docs

Fixes #8

[sindresorhus]

Requires semver major because it changes the default behavior.

Good timing. I already need to do a major release for other things in master.

Approval

👍

index.test-d.ts with null, why is it failing?

I commented inline.

---

Can you fix the merge conflict?

---

I think we should use undefined instead of null.

[fregante]

Null is what querySelector returns, so I thought of keeping it consistent

[sindresorhus]

I’m trying to get rid of null in my packages. We can’t change the browser APIs, but at least we can make our code better.

[fregante]

What’s wrong with null? Is it typeof null === 'object'?

[sindresorhus]

sindresorhus/meta#7

[fregante]

Are only docs missing here?

[sindresorhus]

Yes

[fregante]

Good to go, however we should probably rename it:

• the promise isn't actually cancelled (like .cancel()), but resolved to null
• the "dom ready" event is not commonly known as "dom loaded"

So perhaps it should be stopOnDomReady, untilDomReady, beforeDomReady. The name should reflect that it does indeed stop on dom ready, but it can still be used after the event.

[sindresorhus]

I'm starting to think that .cancel() should be changed too. Since there are many paths that could end up with undefined now, consumers needs to handle it anyway, so .cancel() could just resolve to undefined instead of rejecting. So maybe we should rename .cancel() to .stop() too?

I like stopOnDomReady.

[sindresorhus]

I know you hate this, but I prefer:

(async () => {
	await domLoaded;
	cancelAnimationFrame(rafId);
	resolve();
})();

[fregante]

So maybe we should rename .cancel() to .stop() too?

So I should drop p-cancelable since it wouldn't be a regular cancelable promise.

[sindresorhus]

So I should drop p-cancelable since it wouldn't be a regular cancelable promise.

Yes

[fregante]

Thoughts on this pattern instead?

const elementReady = (selector, {
	target = document,
	stopOnDomReady = true
} = {}) => {
	// Code
};

[sindresorhus]

Sure, that's fine too. It just has a tendency to become kinda messy when you have a lot of options.