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

feat: add Client to named export #264

Closed
wants to merge 1 commit into from
Closed

Conversation

datner
Copy link

@datner datner commented Dec 16, 2023

This will allow importing Client not as default

import { Client } from "pocketbase"

This empowers users who prefer to avoid default exports and allows typing a client through a module declaration instead of interface casting

Solves #263

This will allow importing `Client` not as default
@ganigeorgiev
Copy link
Member

Hm, I think I understand what do you mean but I don't think it is a good idea as I'm against adding another way to instantiate the client. It will add very little benefit with the risk of causing confusion for the end users.

Additionally I believe a very similar outcome to what you want can be achieved without the nondefault export if something like declare class ... is used, aka.:

import PocketBase from "pocketbase";

...

declare class PocketBase {
  collection(idOrName: string): RecordService // default fallback for any other collection
  collection(idOrName: "tasks"): RecordService<Task>
  collection(idOrName: "posts"): RecordService<Post>
}

const pb = new PocketBase("...");
pb.collection("tasks") // this should be RecordService<Task>

But again, the current way to specify types for the record models are more of a temporary solution and will not be needed once we introduce an autogenerated typed client in the future (it still remains a low priority but I'll explore the available options after pocketbase/pocketbase#3271).

@datner
Copy link
Author

datner commented Dec 17, 2023

Unfortunately belief is not enough to make it work, your example is not valid typescript code, and even when fixed it does not do what you think it does 😄
Belief also won't overcome known limitations, the only way to use a declaration in the current state is to hack it.
I really don't think it's confusing to have the client as the default export and as a named import. remember that this is the standard for most of the node ecosystem, including the builtin node api ("node:*"). This has been a standard since the import syntax was introduced in general and commonjs was no longer the only module system around, heck, it even has 2 dedicated tsconfig options! (esModuleInterop, allowSyntheticDefaultImports).
So rest assured, no one will get confused by this.
I understand that a typed client will be coming in The Future ™️ , but this is such a tiny improvement to unlock a more robust way to type the client that does not rely on casting, and it can be done now with a patch release rather than in an unknown amount of time and probably a major release.

Please reconsider, it's a low handing fruit, a free win so-to-speak. I think you might have been a tad quick on the close button haha

@ganigeorgiev
Copy link
Member

I've just tested it locally in a new SvelteKit TS project and it seems to work fine for me.
See also https://www.typescriptlang.org/docs/handbook/declaration-files/by-example.html#classes.

But I don't want to argue on the internet. As mentioned previously for me the benefit of this is very little and introduces a risk for confusion and the need of extra documentation, which I don't want to deal with at the moment.

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.

2 participants