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

No populate on request by default #26

Open
mkrajewski90 opened this issue Aug 30, 2022 · 5 comments
Open

No populate on request by default #26

mkrajewski90 opened this issue Aug 30, 2022 · 5 comments

Comments

@mkrajewski90
Copy link

mkrajewski90 commented Aug 30, 2022

@bluszcz - I have an issue here to resolve (I send it in my next PR) but I back to this PR I think we should no populate by default. I see some proper reasons not to do it:

  1. If Strapi decided not to populate fields by default it means that's their way, so a user should declare populate for a collection.
  2. Populate might be with different types and if it's not default option it's better to give user decisions about populate type (to discuss)
  3. I work on get_data improvement for some collection and I need to add populate option to the method. It doesn't make sense to do populate request for all collections, so this is next reason not to populate by default

My solution

Instead of

def get_document(did)
  uri_document = URI("#{@site.endpoint}/api/#{endpoint}/#{did}?populate=#{populate}")
end

we can have

def get_document(did)
  uri_document = URI("#{@site.endpoint}/api/#{endpoint}/#{did}?#{populate}")
end

with populate method:

def populate
  "populate=#{@config["populate"]}" if @config["populate"]
end

Please let me know what do you think about this.

@mkrajewski90
Copy link
Author

Another question is maybe we should in get_data request store this populate.

So we have two types of requests:

  • collection request (get_data method)
  • resource request (get_document method)

We don't use populate in collection (but we might), but we can just add populate to parameters and it works. But here we must duplicate this in the config (populate for collection, populate for resource) and it might be confusing.

This is an open question to discuss how we should treat populate parameter.

@mkrajewski90
Copy link
Author

Related PR: #27

@bluszcz
Copy link
Owner

bluszcz commented Aug 30, 2022

I think giving option to populate or not is a good idea, also it is good idea to keep the defaults on bare minimum. I will look into it, tests and will provide more feedback. Potentially can be included into next release.

@mkrajewski90
Copy link
Author

mkrajewski90 commented Aug 30, 2022

I think about parametrs for a collection and a resource but this might be overwhelming.

In that case populate goes to parameters.

@bluszcz
Copy link
Owner

bluszcz commented Sep 5, 2022

@mkrajewski90 does your PR resolve this issue fully?

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

No branches or pull requests

2 participants