pocketbase / dart-sdk

PocketBase Dart SDK
https://pub.dev/packages/pocketbase
MIT License
511 stars 51 forks source link

I think it would be better if getFileUrl were placed in RecordModel. #25

Closed its-mario closed 1 year ago

its-mario commented 1 year ago

So let’s say I have an internal model with an imageUrl in it. That look like this:

class Post {
  Post({
    required this.text,
    required this.imageUrl,
  });

  static Post fromRecordModel(RecordModel model, PocketBase base){
    final text = model.getStringValue('text');
    final imageName = model.getStringValue('imageUrl'); // it returns only the name
    final imageUrl = base.getFileUrl(imageName, model);
    return Post(text: text, imageUrl: imageUrl);
  }

  final String text;
  final Uri imageUrl;
}

it has a fromRecordModel constructor. The only way to do it now is to pass pocketBase as an argument. But this feels wrong.

Wouldn’t it be better if I could call getFileUrl directly from the model something like this:

static Post fromRecordModel(RecordModel model){
    final text = model.getStringValue('text');
    final imageName = model.getStringValue('imageUrl'); // it returns only the name
    final imageUrl = model.getFileUrl(imageName);
    return Post(text: text, imageUrl: imageUrl);
  }

I tried to implement it by myself but it seems to be an issue with baseUrl, its accessible only in pocketBase instance. This is what I thought about (but it doesn’t feel right). First, I tried to pass the baseUrl into responseData but then I realized that someone could use a field named baseUrl, and by using it to pass baseUrl will override it. Then I add it as an argument to RecordModel.fromJson(json, baseUrl) method, and this also feels wrong. So the last idea is to have a singleton in which I will write baseUrl when PocketBase is initiated. And then I thought why would not we add a singleton for the entire PocketBase class as I saw in other packages? My question is how would be better?

ganigeorgiev commented 1 year ago

We can't move the getFileUrl() into the RecordModel because the RecordModel is just a DTO and as you noted it doesn't have access to the client instance and base url address.

We can add a helper to return a relative file url but I'm not sure how useful that would be (especially with Dart).

One way to reuse the PocketBase.getFileUrl() method could be also to extend the RecordModel.

And then I thought why would not we add a singleton for the entire PocketBase class as I saw in other packages? My question is how would be better?

I'm sorry. I don't understand the suggestion.

It is up to the developer how they'll structure their code. The SDK is intended to be a generic Dart package to make interacting with the PocketBase API a little bit easier.

Feel free to open a PR if you think there is a better/simpler way to handle it.