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

Collection fixes #24

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

transitive-bullshit
Copy link
Contributor

@transitive-bullshit transitive-bullshit commented Aug 6, 2020

While working on adding support for collections to react-notion, I've come across some high level issues.

  • The current version only ever fetches data for the first collection, but if a page has multiple collections it duplicates asking for the same view for each one instead of the proper view. This is a bug and leads to incorrect data for any page with more than one unique collection.
  • All of the endpoints and logic assume we're talking about tables which is too limiting.
    • Note: this is a breaking change from /table to /collection.
  • Changing block.value.collection.types to block.value.collection.views because it makes a lot more sense.
    • Note: this is a breaking change

Beyond this PR, /collection would ideally not take in a pageId but rather a collectionId and collectionViewId since the abstraction of using a page to fetch a collection doesn't really work once you start having more complicated collections and views. This will also be necessary to fetch the data for a specific collection view dynamically since I don't think it makes sense for the initial page content to contain all the data for all collections and their corresponding views.

So I propose the following going forwards:

  • /page will only contain data for each collection's default view.
  • If the client wants to render non-default views, it would make a call to /collection to fetch that view's data.

@transitive-bullshit transitive-bullshit marked this pull request as ready for review August 6, 2020 16:19
@ThallyssonKlein
Copy link

ThallyssonKlein commented May 4, 2021

image

I'm having this error trying to run this pull request

@transitive-bullshit

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