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 notion as cms #21

Merged
merged 8 commits into from
Feb 27, 2023
Merged

feat: add notion as cms #21

merged 8 commits into from
Feb 27, 2023

Conversation

gabe-frasz
Copy link
Contributor

To address issue #16 and provide guidance for potential solutions, I implemented a few changes:

  • Utilized notion-client to fetch Notion pages
  • Replaced hardcoded markdown and fish files with fetched Notion data

While I could have used the official Notion API, notion-client is a more lightweight option that does not require an API key. Instead, it simply requires the page ID from Notion.

To obtain the page ID, I followed these steps:

  1. Created individual pages on Notion for each Next.js page

Step 1

  1. Added a code block with the appropriate content to each page

Step 2

  1. Enabled "Share to web" and copied the page ID (the last part of the URL)

Step 3

With the page IDs in hand, I passed them to the getCodeBlockFromNotion function to fetch their content.


As this is my first PR, I aimed to thoroughly explain the changes I made and hope they will be helpful.

@vercel
Copy link

vercel bot commented Feb 23, 2023

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

Name Status Preview Comments Updated
fala-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 27, 2023 at 2:46PM (UTC)

@diego3g
Copy link
Owner

diego3g commented Feb 23, 2023

This is great! 🤯

Before reviewing the code I have some doubts about the Notion client you used.

As Notion released their official API, do you have any benchmarks that prove that notion-client is somehow better than the official API?

My fear is about a possible deprecation of this unofficial API in the near future.

@diego3g diego3g self-requested a review February 23, 2023 13:35
@diego3g diego3g added the enhancement New feature or request label Feb 23, 2023
@gabe-frasz
Copy link
Contributor Author

Unfortunately, I cannot guarantee the maintenance of the current client, so it may be best to switch to the official Notion API.

As I was reviewing the documentation, I came across a method that could potentially work for fetching page content based on their ID (the same idea I used at first):

https://developers.notion.com/reference/get-block-children

@diego3g
Copy link
Owner

diego3g commented Feb 23, 2023

That would be great! Can you propose a new version of this PR with the official API?

Also, I can already generate an Notion API key and store it as an environment variable inside Vercel.

@gabe-frasz
Copy link
Contributor Author

That would be great! Can you propose a new version of this PR with the official API?

Also, I can already generate an Notion API key and store it as an environment variable inside Vercel.

Right away!

@gabe-frasz
Copy link
Contributor Author

I've made some changes to use the official Notion API instead of notion-client. The new environment variable is named NOTION_API_KEY.

We can now fetch the page content directly from the API without having to share it to the web. However, we need to link the page to the integration where we got the API key.

Connection

Copy link
Owner

@diego3g diego3g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Just pointed out a suggestion inside the function to get the code from the page.

@diego3g
Copy link
Owner

diego3g commented Feb 24, 2023

Can you please share with me the example page you've used on your Notion so I can duplicate it to mine and create all the pages we need to proceed with the merge of this PR?

Also, I think that we should not put the page IDs inside the code, maybe we could use Vercel Edge Config, what do you think?

Inside Edge Config, we can store key/value variables and access them inside our code during runtime so we can fetch the page IDs from there (you can test this out by deploying this application to your Vercel account with Edge Config enabled).

If you prefer, we can merge this like this and you can create a new PR afterwards to enable Edge Config, what do you think?

Also, in the future it would be great if the application could fetch automatically all the pages from Notion and create new pages inside the explorer, so we won't need to specify each page ID, only the root page ID, so we can fetch all the children pages from the root one and display them in the file explorer (this is for future tho, no need to worry right now).

@gabe-frasz
Copy link
Contributor Author

Notion example page: https://tinted-park-185.notion.site/Fish-801ef0fa04c542ec828881fb41382537

I'm not familiar with Vercel Edge Config, but it seems like an interesting tool. I'll review the documentation and make the necessary updates to the code. I'll provide an update shortly.

@diego3g
Copy link
Owner

diego3g commented Feb 24, 2023

I created the pages inside my Notion workspace, can you check if everything is ok? https://diego3g.notion.site/Fala-Dev-8c5ce35897b740cab1678e176eab1df4

Also, let me know if you want to go with Vercel Edge Config or if you want to do it on a later PR. If so, you can already use the IDs from the pages I sent to you (let me know if you can get the ID or if I need to send it to you).

Also, when ready, I will add the Notion API key to Vercel environment variables.

@gabe-frasz
Copy link
Contributor Author

All of the pages you created in your Notion workspace look good to me. Just remember to connect the integration to the pages so that the Notion API can fetch them successfully. By the way, if you prefer, you can use code as markdown in the setup pages, even though it will be rendered as markdown by shiki anyway.

Regarding the Vercel Edge Config, I have almost finished implementing it. I tested it on my account and it seems to be working fine. You can check out the production demo here

I can get the page IDs, but I need to use the API key of your Notion integration to use them in my Edge Config. If you add the appropriate environment variables, I believe it should work without any issues. The environment variables that I used are NOTION_API_KEY and EDGE_CONFIG.
Here are the steps I followed to create and implement the Edge Config: https://vercel.com/docs/concepts/edge-network/edge-config/get-started

Please let me know if you have any questions or if you want me to go ahead and update the PR.

@diego3g
Copy link
Owner

diego3g commented Feb 24, 2023

Hello @gabe-frasz!

I've already set up the environment variables as you requested so both NOTION_API_KEY and EDGE_CONFIG now are available inside Vercel deployments.

Now that the evnrionment variable is set up, you can submit a commit here and Vercel will be able to set up the preview environment properly.

Can you send me the structure of the data that I need to store inside Edge Config so we can access it properly on the application?

@gabe-frasz
Copy link
Contributor Author

Since the Edge Config feature requested just required storing the Notion pages ID, I created one called notion-pages-id-store and the data is structured as follows:

{
  "general": "",
  "fish": "",
  "devSetup": "",
  "gamingSetup": ""
}

However, it is possible that this Edge Config could be used to store additional data in the future. So feel free to make any modifications you think are necessary and notify me so I can update my code to accommodate them.

@vercel
Copy link

vercel bot commented Feb 24, 2023

@gabe-frasz is attempting to deploy a commit to the Rocketseat Team on Vercel.

A member of the Team first needs to authorize it.

@diego3g
Copy link
Owner

diego3g commented Feb 26, 2023

Hello @gabe-frasz, sorry for the late reply!

I added this structure to Edge Config:

{
  "notion": {
    "terminal_general": "86ebd05238f345f4bc9eb6bd320625e0",
    "terminal_fish": "e6d3fcb0019e4582ae66b825ddf5ee76",
    "setup_dev": "104507af1fe24b7189573a34227e6f85",
    "setup_gaming": "73308e98853c45edb5c724f64b56a4f8"
  }
}

Can you make the code rely on this structure?

@vercel
Copy link

vercel bot commented Feb 26, 2023

Deployment failed with the following error:

Tried to attach Edge Config ecfg_57gaz0taa0s18m6fvjeosvmfswfa, but it does not exist. Remove or update the EDGE_CONFIG environment variable and try again.

Learn More: https://vercel.com/docs/errors#error-list/invalid-edge-config-connection-string

@gabe-frasz
Copy link
Contributor Author

Hello @gabe-frasz, sorry for the late reply!

I added this structure to Edge Config:

{
  "notion": {
    "terminal_general": "86ebd05238f345f4bc9eb6bd320625e0",
    "terminal_fish": "e6d3fcb0019e4582ae66b825ddf5ee76",
    "setup_dev": "104507af1fe24b7189573a34227e6f85",
    "setup_gaming": "73308e98853c45edb5c724f64b56a4f8"
  }
}

Can you make the code rely on this structure?

Up to date!

@diego3g
Copy link
Owner

diego3g commented Feb 27, 2023

Hey @gabe-frasz, i've updated the edge config environment variable on Vercel as I transfered the project to my personal account there. Can you create a new commit just to trigger a new deploy?

@gabe-frasz
Copy link
Contributor Author

I took the time to add an example environment variables file.

Fortunately, it's already working!

@diego3g
Copy link
Owner

diego3g commented Feb 27, 2023

Those are big news! Time to merge! 🚀

@diego3g diego3g merged commit 228e58b into diego3g:main Feb 27, 2023
@gabe-frasz
Copy link
Contributor Author

I'm grateful to have been a part of this and to have contributed in some way. Thank you again for your amazing work in enhancing my knowledge and helping me reach where I am now.

@gabe-frasz gabe-frasz deleted the add-notion-cms branch February 27, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants