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

added API support for i18n, subpages and static field values #372

Merged
merged 6 commits into from
Jan 26, 2024

Conversation

rallisf1
Copy link
Contributor

@rallisf1 rallisf1 commented Jan 23, 2024

This is safe to merge but not final yet. I could use your help @mateomorris !

  • Line 7: the language should be tested against the available languages from the builder, it worked locally but not on Vercel
  • Line 9: Only needed for line 17
  • Line 17: We should also match the parent url in case there are multiple subpages with the same url but different parent

Known bugs:

  1. If you have a 1st-level page with a 2-letter url it will think it's a language code and return no results, also no error

Features under consideration

  1. Markdown fields return both html and markup code, this is not efficient and we should filter it by a ?format= URL query parameter
  2. Remove database ids from response
  3. Add sort and limit URL query parameter for pagination

@rallisf1
Copy link
Contributor Author

rallisf1 commented Jan 24, 2024

A little backstory: I was tired of updating the category or widget components whenever adding a new product or article.

Using the the following component I can load all the blog posts from the blog posts pages via the API. It does slow down deployment, probably hangs when having too many posts but it's ok for me atm.

News.json

The data is fetched during deployment, not on client-side. There's no reason to spam requests to the primo server when the data change only during deployment. I am applying a 5 post pagination, ideally this should be implemented in the API.

Known Bug: If you hit deploy too fast after adding a post it might not be included in the component above, give it a few seconds to be written in the database first.

P.S. Yeah I know I am abusing the Javascript tab by adding a module script in there, but it works :D

@mateomorris
Copy link
Collaborator

mateomorris commented Jan 25, 2024

Line 7: the language should be tested against the available languages from the builder, it worked locally but not on Vercel

Okay I went ahead and exported the languages from primo builder and imported them

Line 9: Only needed for line 17
Line 17: We should also match the parent url in case there are multiple subpages with the same url but different parent

Fixed the parent url matching for sections. looks like you might have been selecting page!inner twice, causing the issue.

Markdown fields return both html and markup code, this is not efficient and we should filter it by a ?format= URL query parameter
Add sort and limit URL query parameter for pagination

Agreed. I don't have more bandwidth atm so we might want to merge it in as in unless you can keep working on it.

Remove database ids from response

Any particular reason you want to remove them? I figured they'd be useful to target specific pages/sections by their ID (for example, to select particular sections on a page without worrying about them being moved around).

@rallisf1
Copy link
Contributor Author

Fixed the parent url matching for sections. looks like you might have been selecting page!inner twice, causing the issue.

Damn, how did I miss that...

Agreed. I don't have more bandwidth atm so we might want to merge it in as in unless you can keep working on it.

Yes, I'll keep working on this.

Any particular reason you want to remove them? I figured they'd be useful to target specific pages/sections by their ID (for example, to select particular sections on a page without worrying about them being moved around).

Just to save bandwidth I guess as I don't use them, but they can stay if they're useful.

How about sort and limit? I think I should add those as well.

@mateomorris
Copy link
Collaborator

Just to save bandwidth I guess as I don't use them, but they can stay if they're useful.

Okay, let's leave them then since they're the only way to absolutely select a page/section.

How about sort and limit? I think I should add those as well.

For sure! What about pagination?


Love the use of the module in the JS tab lol. Does that not run again in the client though?

@rallisf1
Copy link
Contributor Author

rallisf1 commented Jan 25, 2024

For sure! What about pagination?

Limit start,end should take care of pagination and we only need to add a total_pages value somewhere in the response. I'll do a commit tomorrow.

Love the use of the module in the JS tab lol. Does that not run again in the client though?

OMG you are right, I just assumed context="module" scripts would only run server-side. I can't use import { browser } from '$app/environment' in there either...

I worked around that using window but then I stumbled upon the fact the the code would not work in the primo iframe as well. After some messing around it worked in the iframe and the deployment but blog was overwritten to an empty array on production...

Got any trick for that?

@mateomorris
Copy link
Collaborator

I can't use import { browser } from '$app/environment' in there either...

I think '$app/environment' only works in SvelteKit.

I worked around that using window but then I stumbled upon the fact the the code would not work in the primo iframe as well. After some messing around it worked in the iframe and the deployment but blog was overwritten to an empty array on production...

Yeah it looks like when it runs again in production it overwrites the statically rendered content. So I figured that we could output that data as raw json into a hidden div and then pull that instead of fetching data from the API.

const server_host = 'myprimoserver.com'
let blog = []
if (window.location.host === '' || window.location.host === server_host) { // in iframe or primo server (i.e. generating pages)
  blog = await fetchPosts().catch(error => console.error('Error fetching posts:', error));
} else { // in production
  const blog_json = document.querySelector('#blog-json') // get stored data
  blog = JSON.parse(blog_json.innerHTML) 
}

Here's the full Block
News (1).json

@rallisf1
Copy link
Contributor Author

wow, you are hackier than me!

some minor improvements:

let blog = []
if (typeof window === 'undefined' || window.location.host === server_host || window.location.ancestorOrigins[0] === `https://${server_host}`) {
  blog = await fetchPosts().catch(error => console.error('Error fetching posts:', error));
} else {
  const blog_json = document.querySelector('#blog-json')
  blog = JSON.parse(blog_json.innerHTML)
  document.querySelector('#blog-json').remove()
}

P.S. When this PR is merged I'll write a guide on this and call it "Don't Repeat Yourself", it really saves a lot of time and copy-paste errors. I wonder how I didn't try this sooner tbh...

@rallisf1
Copy link
Contributor Author

This isn't extensively tested, but it works. I also moved the sections sorting to the supabase query.

Breaking changes for people already using the API

  1. If you use a 2-letter language code as the url of a top level page for any reason, the API will assume it is a language and won't serve that page or its subpages, without any errors
  2. If you have more than 10 subpages and/or perform your own pagination you need to update your code and use range as a url parameter and look for subpages_total in the response to figure out the pagination

P.S. The API is not strictly typed. Expect errors when you pass bad parameters.
P.S.2. I wish there was a better way to switch between html and markdown but I couldn't think of any.

@mateomorris mateomorris merged commit 1d37327 into primocms:master Jan 26, 2024
@mateomorris
Copy link
Collaborator

Thanks @rallisf1! Looks great

@rallisf1 rallisf1 deleted the api_fixes branch January 28, 2024 11:29
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