-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Move Uploads to constructor and remove enhanceSchema #1204
Conversation
//default we enable them if supported by the integration | ||
} else if (uploads) { | ||
throw new Error( | ||
'This implementation of ApolloServer does not support file uploads.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can make this more specific? We want to make sure people understand it is because of the framework/environment they are using.
@@ -29,6 +29,8 @@ export interface SubscriptionServerOptions { | |||
onDisconnect?: (websocket: WebSocket, context: ConnectionContext) => any; | |||
} | |||
|
|||
// This configuration is shared between all intetgrations and should include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo (integrations)
@@ -439,7 +439,7 @@ describe('apollo-server-express', () => { | |||
} catch (error) { | |||
// This error began appearing randomly and seems to be a dev dependency bug. | |||
// https://github.com/jaydenseric/apollo-upload-server/blob/18ecdbc7a1f8b69ad51b4affbd986400033303d4/test.js#L39-L42 | |||
if (error.code !== 'EPIPE') throw error; | |||
// if (error.code !== 'EPIPE') throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems dangerous to just swallow all errors here. Did you ever figure out why this test throws?
This places uploads configuration inside of the constructor, since the logic is the same across multiple integrations. Including it in the constructor follows the same logic as subscriptions.