tursodatabase / libsql-client-ts

TypeScript/JavaScript client API for libSQL
https://docs.turso.tech/sdk/ts/quickstart
MIT License
226 stars 32 forks source link

Respect proper key runtime #55

Closed Hebilicious closed 1 year ago

Hebilicious commented 1 year ago

Hello, there's a proposal for wintercg runtime keys (ie exports field of package.json), which is currently not very respected in the js ecosystem. Still, I believe that you're using http for vercel only, so it would be nice if you could replace it with the correct vercel key, edge-light, or just add it for backwards compatibility. I think web is generally used for non node runtime, but don't quote me on that. You can find all the keys there https://runtime-keys.proposal.wintercg.org/#edge-light

honzasp commented 1 year ago

Hi, thank you for opening this issue! :) If I understand you correctly, you propose that instead of exposing a range of exports (@libsql/client, @libsql/client/web, ...), this package should use conditional exports to select a correct subset of functionality based on the runtime?

Hebilicious commented 1 year ago

Hi, I'm suggesting that you use the runtime keys specified by the spec in your package.json exports. You would still provide the ranged of exports. For example if you want to indicates that vercel edge runtime users should use the http version of your library, you can do this.

    "exports": {
        ".": {
            "types": "./lib-esm/index.d.ts",
            "import": "./lib-esm/index.js",
            "require": "./lib-cjs/index.js"
        },
        "./edge-light": {
            "types": "./lib-esm/http.d.ts",
            "import": "./lib-esm/http.js",
            "require": "./lib-cjs/http.js"
        },
        "./http": {
            "types": "./lib-esm/http.d.ts",
            "import": "./lib-esm/http.js",
            "require": "./lib-cjs/http.js"
        },
        "./ws": {
            "types": "./lib-esm/ws.d.ts",
            "import": "./lib-esm/ws.js",
            "require": "./lib-cjs/ws.js"
        },
        "./sqlite3": {
            "types": "./lib-esm/sqlite3.d.ts",
            "import": "./lib-esm/sqlite3.js",
            "require": "./lib-cjs/sqlite3.js"
        },
        "./web": {
            "types": "./lib-esm/web.d.ts",
            "import": "./lib-esm/web.js",
            "require": "./lib-cjs/web.js"
        }
    },

And now vercel users should be able to use import { createClient } from "@libsql/client" instead of `import { createClient } from "@libsql/client/http". This is annoying for library authors that we now have so many runtimes (bun, deno, workerd ...) You can find some context of how it affect end users here https://github.com/unjs/nitro/issues/1371

honzasp commented 1 year ago

I think that with this "exports" section, users would need to import @libsql/client/edge-light, not @libsql/client, to get the second import. Maybe you meant a conditional import like this?

"exports": {
    ".": {
         "types": "./lib-esm/index.d.ts",
         "edge-light": "./lib-esm/http.js",
        "import": "./lib-esm/index.js",
        "require": "./lib-cjs/index.js"
    },
    ...
}

However, I'm a bit wary of special-casing every runtime in existence: it seems like a lost battle trying to keep up with every runtime out there :) But you are right that this might be a better developer experience for the runtimes that we do explicitly support. Doug, what do you think? @CodingDoug

CodingDoug commented 1 year ago

I'm a bit wary of special-casing every runtime in existence

I have the same concern. We'll end up with a matrix of options that becomes difficult to document, understand, and maintain. I think it's reasonable to ask the developer to select the desired behavior of the SDK using knowledge of their expected runtime, whatever that may be.

Is there a more straightforward way of making this work automatically for all forseeable situations, without having to commit to staying up to date with the landscape of runtime vendors? @honzasp

honzasp commented 1 year ago

What we can do is to add these conditional exports to the "." export (i.e., what you get when you import "@libsql/client") on a best effort basis.

For example, if we add "edge-light" conditional import that redirects to "./lib-esm/http.js" instead of "./lib-esm/index.js", people who use Vercel Edge Function will be able to import "@libsql/client" and they won't get obscure error messages caused by unsupported Node interfaces. However, people who use other runtimes that we don't explicitly support (such as Alibaba Cloud) will still need to use a specific import, such as import "@libsql/client/web".

In this way, the library would work out of the box with the normal @libsql/client import for most developers, but developers on other runtimes (or using bundlers that don't resolve the conditional imports correctly) would still need to use specialized imports like @libsql/client/web.

Hebilicious commented 1 year ago

@honzasp you're right, that what I meant 🤦🏽

I totally understand and agree that it would be un-reasonable to ask every library out there to do such a thing!

I think a fair way to do this is to add something like browser or web, which tends to be commonly used keys for non-node in your export map.

But since you already have provider specific instructions and distribute multiple versions of your library, I think it's fine to do what @honzasp suggest and add the ones that you explicitly have in your instructions.

This would be great

"exports": {
    ".": {
         "types": "./lib-esm/index.d.ts",
         "edge-light": "./lib-esm/http.js",
         "web": "./lib-esm/web.js",
         "browser": "./lib-esm/web.js",
        "import": "./lib-esm/index.js",
        "require": "./lib-cjs/index.js"
    },
    ...
}

But you can also add all of the one in the spec if you want to be very thorough.

honzasp commented 1 year ago

Yes, I think that would work well. Doug, what do you think? :) @CodingDoug

CodingDoug commented 1 year ago

Sure, if you're comfortable with the maintenance! @honzasp