-
-
Notifications
You must be signed in to change notification settings - Fork 235
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!: cleanup platform support #306
Conversation
Instead of making all of these significant changes in a single gigantic pull request, it might be better to do them as separate ones or at the very least split up the changes into different commits. I'm not Luan, so thankfully I don't have to review this pull request, but I know that I would hate to have to review 494 changed files in one go. As everything is in one massive commit there is no easy way to tell which code changes relate to which of the many listed changes in the pull request description. TL;DR in order to keep Luan mentally sane, I suggest splitting this pull request up into many smaller ones or at the very least into smaller, easier to review commits. |
Took a quick look and |
@absidue Can't split this up into multiple PRs. Every single file was changed to support ESM on node unfortunately. These are basically just changing the imports to include a .js extension. Only files with real changes are:
I will make sure to break these up into separate commits in the future, but do hope these notes help @LuanRT during the review. |
The reason I'm interested in this pull request is that theoretically according to your description, this should make it easier for FreeTube to use YouTube.js. As it's an Electron app and uses webpack, we've had to do a bit more customisation than just adding it to the package.json and importing it. |
You should be able to provide support for any platform you want following this: |
Provide UniversalCache as a wrapper around Platform.shim.Cache.
I'll list our issues with the previous setup and how we got around them.
The most important part for us is number 1, that we'll still be able to import a compiled (ts -> js) but not bundled web build. |
Switching to a more secure vm sounds like a good idea, although preferably one that isn't a native dependency. |
I haven't reviewed this PR completely yet but I'm not sure if we'll be needing that additional VM. I must admit, however, that |
|
@absidue Edit: |
This was an issue within the package.json not specifying the location of the new types. I have fixed this in the last commit. |
Classes like |
It doesn't look like For FreeTube we would want to use the file that |
web exports provide a way to select web implementation manually without relying on the bundler to select it correctly from the "exports" field web points to src/platform/web.js web.bundle points to bundle/browser.js web.bundle.browser points to bundle/browser.min.js agnostic exports provide users of the library to provide their own platform implementation without first importing the default one. agnostic points to src/platform/lib.ts
Thanks for all your changes!!! (hopefully I wasn't too annoying 🙈) |
Uhh okay so the bundle worked but the toDash function is broken. XMLSerializer is probably a better choice than DOMParser for the web build. |
Another breaking change that should be added to the changelog is that various functions now return Promises like Format.decipher and toDash(). |
I can probably revert the format.decipher returning a promise since I removed isolated-vm. Will fix the toDash now. |
toDash seems to be broken in a different way now, it doesn't throw and error but it now returns |
Oops! Fixed now. |
Awesome, just tested again and everything is working as expected. Thanks @Wykerd! And regarding the deno module, this workflow should work:
There's one thing I can't seem to figure out though, how do you get https://deno.land/x to use the |
I am not sure, but I know this something yargs does. |
Seems like you need to create a release tag alongside the normal release which you can then tell deno.land/x to use? |
@Wykerd @LuanRT here are the official docs on how to do it: https://deno.land/add_module |
Thank you @absidue, I did check that and added the webhook to the repo. The problem is that deno.land doesn't have any other additional options, only @Wykerd |
An alternative might be https://nest.land/ which seems to have a CLI to publish packages instead of relying on GitHub webhooks. But setting up deno.land/x will probably be easier in the long run since it wont require manual publishing? |
Although I'm not sure this will work as expected for Deno releases, it will simplify our workflow and allow other maintainers with write-access to release new versions.
Should be good now. |
I'll be merging this today, unless there's more to be done. CC @Wykerd |
3bd855a
to
d49be45
Compare
This brings a cleaner solution to supporting multiple platforms via
PlatformShim
.Internal breaking changes:
require('youtubei.js')
, as configured inexports
of thepackage.json
, but will break users importing specific paths usingrequire
instead ofimport
Dependency changes:
pbkit
, which outputs code with zero runtime dependecies, making the code even more platform agnostic.linkedom
is no longer required in browser runtimes already providing aDOMParser
implementation.Added a more secure JS runtime for node platform usingisolated-vm
. This may be reverted if deemed unnecessary.Deno support is fully realised with types and all via a new build script
build:deno
which outputs a transformed/patched version of thesrc/
directory to a newdeno/
directory. Alsodeno.ts
entrypoint is provided at the top level of the library.The final step for Deno would be to publish to https://deno.land/x. For this I propose:
deno/
directory to a new branch calleddeno
(or similar) upon each release.I do not have any experience with GH actions, so help on this front would be appreciated.