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 observeReadyElements method #36

Merged
merged 23 commits into from
Sep 10, 2021

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Aug 7, 2021

Depends on sindresorhus/type-fest#246


I was unable to use any-observable for these reasons:

  • If I import it at the top level, the user will be forced to install an observable library even if they don't need it.
  • If I import it within the function, I will have to use the import function which will force me to make the observable-returning function a observable-resolving-promise-returning function which would not be user-friendly and a bit annoying to implement.
  • If I try to use import at the top-level in a try/catch block, I will need to require Node.js 14.8. ESLint also has trouble parsing this and making it work is quite inconvenient. Enable Top Level Await eslint/eslint#13178
  • any-observable doesn't use ESM and the interop layer Node.js uses doesn't seem to allow import 'any-observable/register/zen'; to actually register the observer. ESM any-observable#27

When running the unit tests locally, a test for a different part of code that this PR does not change failed:

t.is(promiseStateSync(entireCheck), 'pending', expectation);

Difference:

- 'fulfilled'
+ 'pending'

This error may or may not reappear during runs of the GitHub Action.


Fixes #22


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
@sindresorhus
Copy link
Owner

Wouldn't it be better to use something more platform native and general like async iterable?

Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
index.js Outdated Show resolved Hide resolved
@Richienb
Copy link
Contributor Author

Richienb commented Aug 7, 2021

Wouldn't it be better to use something more platform native and general like async iterable?

Yes it would - changed the code.

@Richienb Richienb marked this pull request as ready for review August 7, 2021 14:16
@Richienb
Copy link
Contributor Author

// @sindresorhus

@sindresorhus
Copy link
Owner

Instead of using .stop(), couldn't you just use break to stop the for await loop?

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Richienb and others added 3 commits September 7, 2021 22:01
Co-authored-by: Sindre Sorhus <[email protected]>
Co-authored-by: Sindre Sorhus <[email protected]>
@Richienb
Copy link
Contributor Author

Richienb commented Sep 7, 2021

Instead of using .stop(), couldn't you just use break to stop the for await loop?

Indeed. Looks like as well as a next() method, I can also specify a return() method.

@sindresorhus
Copy link
Owner

Would be nice to have a test for break usage, and also show usage of break the docs for how to stop the loop, since it might not be immediately obvious to the user.

@Richienb
Copy link
Contributor Author

Richienb commented Sep 8, 2021

Would be nice to have a test for break usage

As far as I can tell, there is no way to test if break; actually propogates to the corresponding internal logic but I'll still add it anyways in case it does and throws an error.

@sindresorhus
Copy link
Owner

As far as I can tell, there is no way to test if break; actually propogates to the corresponding internal logic but I'll still add it anyways in case it does and throws an error.

A test would at least confirm that it correctly stops and resolves with the correct items.

@Richienb
Copy link
Contributor Author

Richienb commented Sep 9, 2021

CI is failing because of sindresorhus/p-state#6

readme.md Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
Richienb and others added 3 commits September 10, 2021 04:06
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
index.js Show resolved Hide resolved
Signed-off-by: Richie Bendall <[email protected]>
@sindresorhus
Copy link
Owner

Regarding #36 (comment). I disagree with your reasoning. The performance difference in real-world scenarios is negligible. But I also don't feel strongly about it and this PR has had enough back-and-forth, so I'm going to leave it.

@sindresorhus sindresorhus changed the title Add observeReadyElements Add observeReadyElements method Sep 10, 2021
@sindresorhus sindresorhus merged commit 9545514 into sindresorhus:main Sep 10, 2021
@sindresorhus
Copy link
Owner

Thanks for contributing this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscribe to new elements on the page
2 participants