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

Clarify populate usage #2034

Merged
merged 19 commits into from
Mar 13, 2024
Merged

Clarify populate usage #2034

merged 19 commits into from
Mar 13, 2024

Conversation

pwizla
Copy link
Collaborator

@pwizla pwizla commented Mar 1, 2024

This PR:

  • (Updated) simplifies the REST API reference documentation for populate, and links to new guides for deeper explanations
  • (New) creates a new guides section for REST API, that could be expanded eventually
    • adds the first 2 guides, both about populating:
      • (New) the first guide is about "Understanding populate" and introduces a new type of docs content which is more about detailed explanations, with diagrams, examples, and side-by-side comparisons for:
        • populating everything 1-level deep
        • populating specific relations:
          • one level deep
          • several levels deep (exemplified by 2 levels)
        • populating components
        • populating dynamic zones:
          • with the shared population strategy
          • with the detailed population strategy
      • (Updated) the second guide is a typical "How to" guide for a very specific use case of populate that mixes populate usage and custom controllers (this is a copy and paste of some previous content, moved to its own guide)

Direct preview links:
👉 Simplified REST API reference for populate
👉 New "Understanding populate" guide
👉 "How to populate creator fields" guide

Copy link

vercel bot commented Mar 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
documentation ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 13, 2024 4:26pm

@pwizla pwizla self-assigned this Mar 1, 2024
@pwizla pwizla added this to the 4.20.5 milestone Mar 1, 2024
@pwizla pwizla added source: Dev Docs PRs/issues targeting the Developer Docs internal PRs created by the Strapi core team pr: new content PRs for new product features or new documentation sections temp: port to v5 Temporary label for PRs that need to be ported to v5 docs labels Mar 1, 2024
@derrickmehaffy
Copy link
Member

Few comments (working from the preview first before I do specific page by page review:

The direct examples are freaking awesome but a few concerns:


For this one: https://documentation-git-dev-clarify-populate-strapijs.vercel.app/dev-docs/api/rest/guides/populate-creator-fields

I have a lot more feedback here as there are several things being done here that I absolutely don't recommend (for performance, security, and as general best practices). But I can help you rewrite a new example.

Here is the quick list:

  • A custom controller shouldn't be used (ideally using a route middleware would be the best)
  • If using a custom controller, the super.find() should be used
  • Finding all then looping through to find each one separately would be a huge performance sink and use a lot of queries to the database
  • Using promise.all() with DB queries will almost certainly cause a ton of performance problems and more likely a deadlock in the database causing the app to crash depending on how many queries are used
  • You shouldn't use strapi.db.query unless absolutely required, strapi.entityService should be used
  • This controller completely removes all sanitization meaning it's quite likely to leak information the user likely wouldn't want leaked

Let me know if you want to do a workshop or just have me create/push a fix

@pwizla
Copy link
Collaborator Author

pwizla commented Mar 5, 2024

Thanks for your feedback, @derrickmehaffy! Really helpful!

  • For columns and "locked" scrolling, the idea was to compare the behavior and returned data. I agree that the current layout is a bit narrow, though 😅 What would you suggest? Should we keep the 2-column layout and modify the CSS to give each column more space, or switch to a classic, 1-column top-down layout? My preference would be for the former as my point really was to compare different syntaxes side-by-side, but happy to discuss it.

  • For the "populate creator fields" this is literally a copy and paste of what we currently have in https://docs.strapi.io/dev-docs/api/rest/populate-select#populating-createdby-and-updatedby. If these are really bad advices we should take it down a.s.a.p, and I'd be happy to have a workshop with you so that I can understand and write better instructions.

For this example, we should also show the "complex" population (object syntax) that we use here

I'm not sure to fully understand what we should do 🤔 Is this yet another use case — if yes, I should probably add it to the table here as well and create another section. Maybe we could discuss this during our workshop too? 😊

@derrickmehaffy
Copy link
Member

Hey @pwizla I'm discussing with @kasonde and we think the populate syntax used also might be wrong in a few locations but we want to run some direct testing to confirm.

How soon are you trying to merge this so Rich and I can do some QA testing?

@pwizla
Copy link
Collaborator Author

pwizla commented Mar 5, 2024

How soon are you trying to merge this so Rich and I can do some QA testing?

Thanks for your help, @derrickmehaffy and @kasonde! Ideally we'd like to have this live in docs.strapi.io for end of March (as it's a project that should be completed in Q1 2024).

@pwizla pwizla modified the milestones: 4.20.5, 4.20.6 Mar 13, 2024
@derrickmehaffy
Copy link
Member

@pwizla I rewrote that populate example, here is the preview: https://documentation-git-dev-clarify-populate-strapijs.vercel.app/dev-docs/api/rest/guides/populate-creator-fields

@pwizla pwizla merged commit 49c78ce into main Mar 13, 2024
2 of 3 checks passed
@pwizla pwizla deleted the dev/clarify-populate branch March 13, 2024 16:26
@pwizla pwizla removed the temp: port to v5 Temporary label for PRs that need to be ported to v5 docs label Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal PRs created by the Strapi core team pr: new content PRs for new product features or new documentation sections source: Dev Docs PRs/issues targeting the Developer Docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants