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

Use p-cancelable #91

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Use p-cancelable #91

merged 1 commit into from
Apr 24, 2024

Conversation

longnguyen2004
Copy link
Collaborator

This is an implementation of my suggestion in #88 (comment)

Usage:

let playback;
try {
    playback = streamLivestreamVideo(...);
    await playback;
}
catch (e)
{
    if (playback.isCanceled)
        // do stuff when cancel
    // do stuff when error
}

// somewhere else
playback?.cancel();

Copy link
Owner

@dank074 dank074 left a comment

Choose a reason for hiding this comment

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

I like this, definitely a lot cleaner than before

@dank074 dank074 merged commit c696d16 into dank074:master Apr 24, 2024
@longnguyen2004
Copy link
Collaborator Author

I just realized p-cancelable is ESM...If we were to go this route then this library needs to be changed to ESM also

@KuntilBogel
Copy link

@longnguyen2004 iirc you can use dynamic import at commonjs

@longnguyen2004
Copy link
Collaborator Author

I don't really want to use dynamic imports because it needlessly complicates logic for simple use cases, I'd rather have everything be ESM or CommonJS.
I'm in the process of converting this library to ESM. Hope it won't be too much of a breaking change to users.

@KuntilBogel
Copy link

@longnguyen2004 but iirc changing the package from cjs to ejs would affect cjs users, am I right?

@longnguyen2004
Copy link
Collaborator Author

It'll be a breaking change yes, but the situation now is a lot better than when ESM was first added to Node a few years ago, and there shouldn't be many cases where you have to use CJS anymore.
Also, people who are using TypeScript are already using ESM syntaxes, but aren't aware that TS transpiles it to CJS by default. Switching to ESM for such people will be even easier.

@KuntilBogel
Copy link

alr

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.

3 participants