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(graphql): Allow including 'File' scalar by default to be disabled #11540

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

Josh-Walker-GM
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM commented Sep 13, 2024

There was a problem introduced in v8 when we included the File scalar by default. This meant a custom implementation by the user could be clobbered by the new default. This change allows the user to supply config to disable including it by default.

This is not how I would have loved to have done things here. Config in two places is rubbish but given the organisation of this currently it was generally unavoidable.

@Josh-Walker-GM Josh-Walker-GM added the release:fix This PR is a fix label Sep 13, 2024
@Josh-Walker-GM Josh-Walker-GM added this to the next-release-patch milestone Sep 13, 2024
db.$disconnect()
},
// highlight-start
includeScalars: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is better than what I had - the config is at least consistent looking between toml and Yoga config. Nice.

Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

Thanks for this! @Josh-Walker-GM

LGTM and will now let people who have had a Prisma File model opt out effectively of Uploads.

My only question is if in The Default Scalars docs or in Storage we should mention that if you opt out of the file scalar, your project will not be able to use Redwood Storage or any GraphQL file upload.

Tobbe
Tobbe previously requested changes Sep 13, 2024
Copy link
Member

@Tobbe Tobbe left a comment

Choose a reason for hiding this comment

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

Can we please discuss this one more round before merging?

@cannikin
Copy link
Member

Love that this is still enabled by default and it's opt out!

@Tobbe Tobbe dismissed their stale review September 17, 2024 18:14

Letting others decide

@Josh-Walker-GM
Copy link
Collaborator Author

We decided we wanted to go ahead with this as a fix for the reported problem.

@Josh-Walker-GM Josh-Walker-GM merged commit 0081d3a into main Sep 18, 2024
50 checks passed
@Josh-Walker-GM Josh-Walker-GM deleted the jgmw/fix-graphql-scalar-config branch September 18, 2024 17:36
Josh-Walker-GM added a commit that referenced this pull request Sep 19, 2024
…#11540)

There was a problem introduced in v8 when we included the `File` scalar
by default. This meant a custom implementation by the user could be
clobbered by the new default. This change allows the user to supply
config to disable including it by default.

This is not how I would have loved to have done things here. Config in
two places is rubbish but given the organisation of this currently it
was generally unavoidable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants