Skip to content
This repository was archived by the owner on Jul 28, 2024. It is now read-only.

feat: move logic from generate.js to api-endpoint#928

Merged
eddiejaoude merged 1 commit intonextjs-livefrom
cahllagerfeld/generate-to-api
Dec 20, 2021
Merged

feat: move logic from generate.js to api-endpoint#928
eddiejaoude merged 1 commit intonextjs-livefrom
cahllagerfeld/generate-to-api

Conversation

@Cahllagerfeld
Copy link
Copy Markdown
Member

@Cahllagerfeld Cahllagerfeld commented Dec 18, 2021

closes #923

Definition of Done

  • remove generate.js
  • remove generate script
  • move logic from existing file to api-endpoint

Copy link
Copy Markdown
Member Author

@Cahllagerfeld Cahllagerfeld left a comment

Choose a reason for hiding this comment

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

pointed out to spots which are a bit fragile, but worked in my tests

const relativeDir = path.resolve("./public", "profiles");
const files = fs.readdirSync(relativeDir);

const profiles = await Promise.all(
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is a bit fragile imo. If one Promise fails, Promise.resolve wont resolve successfully. I think a propper error-handling needs to be added here. Its discussable if with this, or a future PR.

const profiles = await Promise.all(
files.map(async (file) => {
const response = await fetch(
`http://localhost:3000${path.join("/", "profiles", file)}`
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We should maybe think of a solution of not hardcoding the URL although it needs to be a absolute one.
(Afaik, in index.js the Request-URL is also hardcoded.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think if a request url the join is not required, only if it is a disk path

I wonder if we can use a relative url here? /profiles/...

@eddiejaoude
Copy link
Copy Markdown
Member

I am going to merge this and more work can continue in the nextjs-live branch (or via another PR if preferred)

@eddiejaoude eddiejaoude merged commit 5f5a254 into nextjs-live Dec 20, 2021
@eddiejaoude eddiejaoude deleted the cahllagerfeld/generate-to-api branch December 20, 2021 22:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants