-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Deno compatibility #34
Comments
On your last point, you can join the Revolt Lounge server and discuss this in the Revolt Development channel. |
Ok, this seems to be more work than I initially expected: To maintain compatibility with Node, I tried to make a global deps.ts file with import()'s so that we could use URLs for Deno and NPM packages for Node. It ended up being a mess, with every module declaration being lost. That's not a fault of Deno; that's a fault of mine, because I have NO idea how to organize anything. Also, while '.ts' is required in Deno, it would absolutely break Node. We'd have to use |
This should be a little easier once Node 18+ becomes the norm/more widespread as it supports built-in fetch via undici |
I was told the dev team didn't want to replace axios, so we'll see what happens with that. |
A lot of dependencies on Axios have been untied and it's likely going to be stripped out in the near future, it pretty much entirely on lives within the |
Alright, I'll have to join the Revolt channel again and discuss this. It's been a while since I've been on Revolt, either way, because this was one of the big factors for me. |
Currently halting on denoland/deno#7296 so we don't get the monstrosity that is the comment I made above: #34 (comment) |
Hm, so with Deno getting official NPM support and skypack, it may be worthwhile to reconsider whether this is needed or not. Obviously nothing will replace a decentralized module that wouldnt run through a compat layer, but it may be more work than its worth to introduce cross Node and Deno compatibility. I personally no longer use Revolt, simply because the servers that were initially pro-Revolt decided against having a bridge, meaning im stuck on a different platform. |
For what it's worth, if the lib isn't compatible with Deno yet I'm still in favour of compatibility |
So I just discovered the hard way that it is not compatible with Deno yet! Keep up the good work! ❤️ |
On the first point, I plan to drop axios in the near future: insertish/oapi#2 |
Importing revolt.js using Deno's npm compatibility almost works, but it's broken due to #83. Fixing the second point mentioned in this issue is possible if we use esbuild instead of tsc to build Node releases. Dropping Axios support would be nice as that'd allow revolt.js to run without the compatibility layer included in Deno but that'd take a bit more work. |
I’ve been looking into Deno for the past few days and, from what I can tell, it’s quite hard to achieve, largely due to what I’m guessing is the Deno LSP and its handling of indexed/lookup types and the underlying revolt-api package. I initially tried switching the official package to simply using fetch instead of axios before moving to a custom build script and eventually dropping openapi-typescript completely. The test package is on JSR here with the main source available here. Both the initial version of that package and williamhornings revolt-api-types here use indexed types, as jsr docs show, causing all types to break when they’re added to a new project, while working flawlessly when working from the same directory. Moving the builder to outputting 'real' types based off of the schema fixed type imports and JSR doc generation, as seen and testable with this version, however still keeping the fetch client mostly untyped. After moving to generating the routes with previously generated types directly the issue still occurs when using Deno and its LSP while being perfectly functional with Bun. ReproduceThe following snippet was tested on Linux and MacOS, using latest Deno and the Deno VSCode Extension. import { API, type Message } from "@rawrxd/revolt-api";
const client = new API({
authentication: {
revolt: "totallyarealtoken"
}
});
let data = client.get(`/users/@me`);
const message: Message = {}; Expected
Actual BehaviorOn Deno, using Potential CauseUsing "Go to Definition", Deno correctly identifies The same issue appears on the API client, with Bun resolving both the Testing this even further it seems like Deno or its LSP is unable to resolve indexed types specifically when they're coming from remote packages. Importing the entrypoint of my JSR package via the file system correctly types everything while adding the remote package simply returns StatusAlthough Deno is technically compatible as it successfully runs the example request its developer experience is massively degraded due to the lack of types, with the built-in TypeScript support likely being one of the main reasons to use Deno in the first place. In the current state getting Deno fully compatible with revolt-api alone would likely require a fairly large amount of work and completely dropping or somehow working around indexed types, if that's even possible to reasonably achieve in the first place. |
the way that Deno's LSP works here is not ideal and it makes developing solutions around autogenerated code. I've experimented a fair bit with the revolt-api package as part of getting it to play nicely with lightning, and with the way things are right now, there's not a nice way to get autogenerated code from openapi types to work without investing a lot of work either in the codegen itself or in the Deno LSP
completely dropping indexed types would introduce a lot more complexity with the code generation and the changes would need to take place in oapi, which relies a lot on the way indexed types work to function. imo, this is an issue with the Deno LSP itself, as this use of indexed types works as expected everywhere else (Bun, Node, etc.), but I'm not too sure how to fix that or where to start since I don't have a ton of experience with Rust. it's probably more feasible, at least in terms of work, to try and find some way to work around this |
It would be nice if we could use this within Deno. Here are the things that would have to occur if we were to proceed with this:
There are many reasons to use Deno over Node, including the built-in TypeScript support, Permission system and much more
Is there a Revolt channel I could use to discuss revolt.js? It would be best for fast communication
The text was updated successfully, but these errors were encountered: