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

Add `elementReady.subscribe()` #24

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@qti3e
Copy link

commented Jul 1, 2019

This approach uses a WeakMap to store already seen elements (to prevent memory leak) in order to detect new elements, also because checking all the elements to find out new ones might be a bit heavy we do this in half of the requestAnimationFrames, feedback is welcome.

Issue: #22


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@qti3e qti3e force-pushed the qti3e:q/sub branch from c904bd9 to d74eea4 Jul 1, 2019

Show resolved Hide resolved index.js Outdated
Show resolved Hide resolved index.js Outdated
*/
stop(): void;
};

type StoppablePromise<T> = Promise<T> & {

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jul 1, 2019

Collaborator

Perhaps, but the description doesn't match.

 	type StoppablePromise<T> = Promise<T> & Stoppable;

This comment has been minimized.

Copy link
@qti3e

qti3e Jul 1, 2019

Author

I actually wanted to do so, but I didn't want to change the doc comments.

@qti3e

This comment has been minimized.

Copy link
Author

commented Jul 7, 2019

@bfred-it Hey! I've made a small change and I guess this PR is ready to be merged.
Please let me know your opinion.

index.js Outdated
if (checkFrame) {
const allElements = target.querySelectorAll(selector);

for (let i = 0; i < allElements.length; ++i) {

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner

Use a for-of loop.

index.js Outdated
@@ -54,4 +54,52 @@ const elementReady = (selector, {
return Object.assign(promise, {stop});
};

elementReady.subscribe = (selector, cb, {

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner

Don't use abbreviations. cb => callback.

@@ -12,4 +12,17 @@ expectType<elementReady.StoppablePromise<Element | undefined>>(promise);
expectType<elementReady.StoppablePromise<HTMLDivElement | undefined>>(elementReady('div'));
expectType<elementReady.StoppablePromise<SVGElement | undefined>>(elementReady('text'));


const {stop} = elementReady.subscribe(".unicorn", e => null);
elementReady.subscribe(".unicorn", e => null, {target: document});

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner

Use single-quotes

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jul 7, 2019

Collaborator

Why doesn’t xo catch this?

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner

XO doesn't run on TS files unless you enable it explicitly. xojs/xo#373 will enable us to automatically lint .d.ts files too.

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jul 7, 2019

Collaborator

Indeed, I meant to say "let's just enable XO for TS files on this repo"

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner

Nah, I try to keep my boilerplate config as identical as possible between repos for ease of maintenance. I'd rather wait for XO.

readme.md Outdated

Same as `elementReady` options.

#### elementReady.subscribe()#stop()

This comment has been minimized.

Copy link
@sindresorhus
test.js Outdated
@@ -187,3 +187,56 @@ test('ensure different promises are returned on second call with the same select
document.querySelector('.unicorn').remove();
t.is(prependElement(), await elementReady('.unicorn'));
});

test('subscribe: check if elements are detected', async t => {
const e = () => {

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner

Don't use one character variable names.

### elementReady.subscribe(selector, cb, options?)

Detect elements as they are added to the DOM.

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner

Include some use-cases for this method.

index.d.ts Outdated
/**
Called on each new element that matches the selector in the DOM.
*/
type SubscribeCallback<T> = (el: T) => any;

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner

This type is incorrect. According to the JS code, the return value is not used.

Suggested change
type SubscribeCallback<T> = (el: T) => any;
type SubscribeCallback<T> = (element: T) => any;
index.d.ts Outdated
Detect elements as they are added to the DOM.
@param selector - [CSS selector.](https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Getting_Started/Selectors)
@param cb Callback that is called for each new element.

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner
Suggested change
@param cb Callback that is called for each new element.
@param cb - Callback that is called for each new element.
index.d.ts Outdated
/**
Stop checking for new elements.
Calling it multiple times does nothing.

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner

Incorrect indentation.

index.js Outdated
stopOnDomReady = true,
timeout = Infinity
} = {}) => {
const seen = new WeakSet();

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Jul 7, 2019

Owner
Suggested change
const seen = new WeakSet();
const seenElements = new WeakSet();
@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jul 7, 2019

The implementation looks good, but I think you need to put more effort into the docs and TS definition.

@sindresorhus sindresorhus changed the title Implement elementReady.subscribe Add `elementReady.subscribe()` Jul 7, 2019

@qti3e

This comment has been minimized.

Copy link
Author

commented Jul 8, 2019

@sindresorhus Sorry for all of this style-guide related issues, I wasn't quite familiar with your coding guide-lines, anyway I've addressed all of your comments.

Also, I made a change to SubscribeOptions, I think SubscribeOptions.stopOnDomReady's default is better to be false as when a user calls this function they want to subscribe to all of the new elements, also they're probably going to use this function after the DOM ready event most of the time.

@qti3e qti3e force-pushed the qti3e:q/sub branch from 1f83646 to a6f2f88 Jul 8, 2019

@qti3e qti3e force-pushed the qti3e:q/sub branch from a6f2f88 to 33b5311 Jul 8, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

  1. My usage would be exactly to catch elements as the page loads, until the DOM ready
  2. It’s best to match the options for the regular elementReady
@qti3e

This comment has been minimized.

Copy link
Author

commented Jul 18, 2019

ping @bfred-it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.