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

Fix file upload in nextjs-keystone example #8179

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

marekryb
Copy link
Contributor

@marekryb marekryb commented Dec 18, 2022

Fixes #8176

@changeset-bot
Copy link

changeset-bot bot commented Dec 18, 2022

🦋 Changeset detected

Latest commit: c8779bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@keystone-6/example-nextjs-keystone Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Dec 18, 2022

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

Name Status Preview Comments Updated
keystone-next-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 28, 2023 at 0:58AM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 18, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 58ab9a6:

Sandbox Source
@keystone-6/sandbox Configuration

@vercel vercel bot temporarily deployed to Preview December 18, 2022 08:54 Inactive
@marekryb
Copy link
Contributor Author

@flexdinesh There is one unsolved issue. In keystone.ts / generateUrl there is hardcoded port 4000. This will work fine for 'yarn next:dev', but will not work for 'yarn keystone:dev' (port 3000). Normally I would pass this as some env variable, but not sure how to do this in this example without complicating things. Some guidance please.

@flexdinesh flexdinesh self-requested a review December 18, 2022 23:56
@@ -19,7 +19,7 @@ const permissions = {
export const lists: Lists = {
User: list({
// readonly for demo purpose
access: permissions.readOnly,
access: allowAll,
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably have to change the permission to readOnly. This example is deployable to vercel in one click using the button in the docs by anyone. allowAll gives CRUD access to all fields and it's probably not safe for an example deployed on vercel with public write access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

my_images: {
kind: 'local',
type: 'image',
generateUrl: path => `http://localhost:4000/images${path}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

We might have to set the URL using environment variables. Apps deployed on vercel will have their URL defined in the env variable process.env.NEXT_PUBLIC_VERCEL_URL.

@flexdinesh
Copy link
Contributor

@marekryb re: generateUrl there is hardcoded port 4000

That's a tricky one. I think we could use environment variables to identify which server we're running (keystone or next). Perhaps we could replace the current next:dev script with "next:dev": "PORT=4000 next dev" and use the PORT env variable to conditionally setup the localhost URL.

But this probably adds an unwarranted layer of complexity to a simple example. Maybe we should just create a separate example to showcase file uploads with nextjs-keystone example and leave this example be as simple as it could be.

@vercel vercel bot temporarily deployed to Preview January 28, 2023 12:55 Inactive
@vercel vercel bot temporarily deployed to Preview January 28, 2023 12:58 Inactive
@marekryb
Copy link
Contributor Author

marekryb commented Jan 28, 2023

@flexdinesh I added passing public url through env variable (through cross-env as otherwise someone will come and complain that it doesn't work under Win). Vercel url is prioritized.

See how you like the changes. Alternatives are:

  1. split this to separate example,
  2. have two graphql apis - one plain and one with upload (upload one can be commented out),
  3. leave 'processRequest' block as commented alternative and don't change schema at all.

PS. This example does not work well at the moment due to #8278 (unrelated) ;-)

PS2. Currently there is some random lint error when building through next:build (but not keystone:build). It happens also on main branch (with no changes). Not sure what is happening...

@mmachatschek
Copy link
Contributor

@borisno2 this fixes the issue with file uploads and graphql-yoga for me.

If someone imports keystone in a different project (workspace setup), you need to make sure to export the processRequest from the keystone project and not adding graphql-upload to the dependencies of the other project (because of instanceof checks in graphql-upload).

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.

File upload does not work in 'keystone-in-nextjs' example
4 participants