-
Notifications
You must be signed in to change notification settings - Fork 65
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
feat(schema): add schema.middleware #688
Conversation
Weakky
commented
Apr 21, 2020
- docs
- tests
756045d
to
3e8406a
Compare
@@ -83,7 +85,7 @@ export function create(): App { | |||
} | |||
|
|||
const server = Server.create() | |||
const schemaComponent = Schema.create() | |||
const schemaComponent = Schema.create(__state) |
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.
Interesting, I had being going a direction before where schema would have its own state. Your approach reminds me of a react-ish atomic-state idea. Any particular thoughts here or you just did what first came to you?
@@ -5,7 +5,7 @@ import { mapSettingsToNexusSchemaConfig } from './settings' | |||
let schema: ReturnType<typeof create> | |||
|
|||
beforeEach(() => { | |||
schema = create() | |||
schema = create({ isWasServerStartCalled: false, plugins: [] }) |
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.
Does this part feel like a leak of detail?
const middleware: SchemaInternal['public']['middleware'] = (fn) => { | ||
api.public.use( | ||
NexusSchema.plugin({ | ||
// TODO: Do we need to expose the name property? |
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.
no?
// todo if caleld after app start, raise a warning (dev), error (prod) | ||
// this will require the component having access to some of the | ||
// framework state. | ||
if (appState.isWasServerStartCalled === true) { |
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.
Ahhh this is the key reason to pass down app state, I see. Def. a fair point!
Co-Authored-By: Jason Kuhrt <[email protected]>
Co-Authored-By: Jason Kuhrt <[email protected]>