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

refactor!: better cross runtime support #97

Merged
merged 89 commits into from
Jul 20, 2022
Merged

refactor!: better cross runtime support #97

merged 89 commits into from
Jul 20, 2022

Conversation

Wykerd
Copy link
Collaborator

@Wykerd Wykerd commented Jul 11, 2022

Description

This builds on efforts of #91 and intends to make the library work on browsers and Deno (possibly CFWorkers) by replacing as much dependencies as possible with equivalent Web APIs available on all platforms.

This is a major change which will break event and stream APIs as these are to be replaced by their Web API equivalents.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Changes:

  • Remove node APIs
    • crypto -> SubtleCrypto
    • stream -> Web Stream API
    • events -> EventTarget
    • make sure buffer is polyfilled on all platforms
  • Replace Axios with Fetch API - Fetch is available on more platforms than XMLHTTPRequest
  • Documentation for Deno usage
  • Update tests for browsers
  • Documentation for browser usage
  • Tests for Deno?

EDIT: will not support Bun.js at this time, too experimental at this moment.

removes node-forge and uuid in favor of Web APIs
To aid with #93 I will make all my changes in TypeScript instead.
This is the first step into making that happen.

Used: https://github.com/wessberg/cjstoesm
Copy link
Collaborator Author

@Wykerd Wykerd left a comment

Choose a reason for hiding this comment

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

I now realize that this is going to cause massive merge conflicts with #93, but how bad could it be 😅

Bring this PR up to speed with #93
@LuanRT
Copy link
Owner

LuanRT commented Jul 11, 2022

events -> EventTarget

Maybe we could work on a custom implementation of EventEmitter? It seems quite simple and would probably be better compared to the verbosity & weirdness of the EventTarget API.

@LuanRT
Copy link
Owner

LuanRT commented Jul 11, 2022

I now realize that this is going to cause massive merge conflicts with #93, but how bad could it be 😅

Haha there's already a quite big conflict due to 68cb841 (which was an inevitable change as we had lots of dead code laying around). I guess we could help but unfortunately he's working on a branch outside of the project.

this is untested!
should remove idb as dependecy.
Copy link
Collaborator Author

@Wykerd Wykerd left a comment

Choose a reason for hiding this comment

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

How about this for EventEmitter? Best of both worlds haha

Copy link
Owner

@LuanRT LuanRT left a comment

Choose a reason for hiding this comment

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

Awesome!

Wykerd added 10 commits July 12, 2022 16:02
Initial TS support for parsers as per #93

This adds several type safety checks to the parser which'll help to
ensure valid data is returned by the parser.
Bring more in line with the existing implementations & make less verbose
I was overcomplicating things, this is much simpler and compatible with
the existing JS API
Again, this also does some work for #93
@Wykerd Wykerd added this to the v2 milestone Jul 13, 2022
@Wykerd
Copy link
Collaborator Author

Wykerd commented Jul 13, 2022

I'm going quite out of the scope of this PR by doing so much TS conversion too, but I cannot write my new changes in TS without then also converting some of the other parts of the project to TS too.

@LuanRT
Copy link
Owner

LuanRT commented Jul 13, 2022

I'm going quite out of the scope of this PR by doing so much TS conversion too, but I cannot write my new changes in TS without then also converting some of the other parts of the project to TS too.

At this point this probably supersedes @MasterOfBob777's PR.

@Wykerd
Copy link
Collaborator Author

Wykerd commented Jul 13, 2022

I'm going quite out of the scope of this PR by doing so much TS conversion too, but I cannot write my new changes in TS without then also converting some of the other parts of the project to TS too.

At this point this probably supersedes @MasterOfBob777's PR.

There's no way I'm rewriting all the parsers to TS in this PR though, so there's still a lot of work to be done if we want the entire codebase to be TS, for now I've enabled allowJS in the tsconfig

LuanRT added a commit that referenced this pull request Jul 13, 2022
Also, remove “strict” rule in favor of typescript (#93, #97)
@BobVarioa
Copy link
Collaborator

Wow! This is very cool! As @Wykerd mentioned I do think my PR has its place, but I think it is probably best to wait until this PR is completed to avoid merge conflict hell.
I also just want to mention it might be better to compile for each platform separately with esbuild constants, instead of programmatically figuring out the runtime, as it increases bundle size by quite a bit if you include unnecessary code in their respective platforms (Also the exports field in package.json can also achieve a similar effect by swapping out modules by path.) Regardless this isn't a huge issue at the end of the day, because it would barely effect performance, as V8 and other engines should just realize those branches are impossible to reach and "remove" the check, and basically achieve the same thing.
Those are just my two cents though it might not be the right decision in this case.

@LuanRT LuanRT added the enhancement New feature or request label Jul 13, 2022
@LuanRT
Copy link
Owner

LuanRT commented Jul 17, 2022

I think Innertube#session.signIn() doesn't wait for the authentication to finish.

Ex:

//...

// Triggers the auth event and asks the user to authorize the authentication.
await yt.session.signIn(undefined); 

// This will return true even though authorization hasn't been granted yet.
// In fact, this should not be printed at all since the auth flow hasn't finished and any InnerTube call will inevitably fail.
console.log(yt.session.logged_in);

@Wykerd
Copy link
Collaborator Author

Wykerd commented Jul 17, 2022

I think Innertube#session.signIn() doesn't doesn't wait for authentication to finish.

Ex:

//...

// Triggers the auth event and asks the user to authorize the authentication.
await yt.session.signIn(undefined); 

// This will return true even though authorization hasn't been granted yet.
// In fact, this should not be printed at all since the auth flow hasn't finished and any InnerTube call will inevitably fail.
console.log(yt.session.logged_in);

It does not, it only waits until the first auth event is fired with the login code

@Wykerd
Copy link
Collaborator Author

Wykerd commented Jul 17, 2022

Should I make it wait instead? Probably makes more sense if it does.

@LuanRT
Copy link
Owner

LuanRT commented Jul 17, 2022

Should I make it wait instead? Probably makes more sense if it does.

Yup, not doing so would probably cause some confusion.

Copy link
Collaborator

@BobVarioa BobVarioa left a comment

Choose a reason for hiding this comment

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

Just a few small comments I had. Nothing that seem like a huge issue, or a glaring bug, but since this a large PR I most definitely missed something, and frankly I don't have a large amount of experience with the library.

lib/Innertube.ts Outdated Show resolved Hide resolved
lib/core/PlaylistManager.ts Show resolved Hide resolved
lib/core/PlaylistManager.ts Show resolved Hide resolved
lib/parser/README.md Outdated Show resolved Hide resolved
lib/utils/Constants.ts Show resolved Hide resolved
lib/utils/HTTPClient.ts Outdated Show resolved Hide resolved
lib/utils/Utils.ts Show resolved Hide resolved
lib/utils/Utils.ts Show resolved Hide resolved
lib/utils/Utils.ts Show resolved Hide resolved
lib/utils/Utils.ts Outdated Show resolved Hide resolved
This also splits the 'auth' event up into 3 distinct events:
- 'auth' -> fired on success
- 'auth-pending' -> fired when pending authentication
- 'auth-error' -> fired when an error occurred
@Wykerd
Copy link
Collaborator Author

Wykerd commented Jul 17, 2022

Just a few small comments I had. Nothing that seem like a huge issue, or a glaring bug, but since this a large PR I most definitely missed something, and frankly I don't have a large amount of experience with the library.

I do think there will be some small bugs that slipped through the cracks here and there, especially in the JS code. They should be easy to find once the project is completely migrated to TS.

@Wykerd Wykerd assigned Wykerd and unassigned Wykerd Jul 17, 2022
@Wykerd
Copy link
Collaborator Author

Wykerd commented Jul 17, 2022

Whoops assigned this to myself accidentally 🤣

@Wykerd Wykerd requested a review from LuanRT July 18, 2022 16:39
@Wykerd
Copy link
Collaborator Author

Wykerd commented Jul 18, 2022

I think this PR is ready to be merged now. Mind giving it a look @LuanRT ?

index.ts Outdated Show resolved Hide resolved
@LuanRT
Copy link
Owner

LuanRT commented Jul 20, 2022

This should be item_type, MusicResponsiveListItem represents one single item inside a list, and not a list. It may also be mixed with other items (ex; in YTMusic's home feed).

this.list_type = ('video');

@LuanRT
Copy link
Owner

LuanRT commented Jul 20, 2022

I don't see any other major bug, so merging now. Thanks @Wykerd!

@LuanRT LuanRT merged commit fb68e6b into main Jul 20, 2022
@Wykerd Wykerd deleted the Wykerd-deno-support branch July 21, 2022 06:24
@LuanRT LuanRT changed the title Better cross runtime support refactor!: better cross runtime support Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants