-
Notifications
You must be signed in to change notification settings - Fork 2k
Rename 'MaybePromise' to 'PromiseOrValue' #1798
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
Conversation
Suggested by @martijnwalraven `MaybePromise` name confuses users into thinking: `execute`/`graphql` returns either Promise or null/undefined. Also in our codebase we use `[Mm]aybe*` to represent optional things, e.g.: https://github.com/graphql/graphql-js/blob/8c96dc8276f2de27b8af9ffbd71a4597d483523f/src/language/visitor.js#L353 https://github.com/graphql/graphql-js/blob/8c96dc8276f2de27b8af9ffbd71a4597d483523f/src/language/printer.js#L244
@Cito What do you think? Maybe you have a better naming proposal? |
Note: not breaking change, since it's part of
|
The name MaybePromise is also used by others, but @martijnwalraven has a point when he says Maybe already means "or null/undefined" in GraphQL.js elsewhere, and Flow also has the concept of Maybe Types for optional values. PromiseOrValue sounds good to me. I found that name also in the typescript definition for Promise.all(). So renaming maybe (pun intended) makes sense. |
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.
This seems like a solid clarity win to me, so I'm all for it.
Thanks for research 👍 |
* Add Martijn Walraven to "Definitions by" section of the package header * Add `validateSDL` function * Add `toConfig` methods See graphql/graphql-js#1331 * Add missing typing for `print` function See graphql/graphql-js#1702 * Rename `MaybePromise` to `PromiseOrValue` graphql/graphql-js#1798 * Rename `blockStringValue.js` to `blockString.js`, and `blockStringValue` to `dedentBlockStringValue` See graphql/graphql-js@16afd2e and graphql/graphql-js@60a2bf8 * Update `graphql` version to 14.2 * Add `.prettierrc` and reformat all files This adds a `.prettierrc` based on the defaults used in #24552 (comment), and reformats all files (only a few are affected). * Fix `GraphQLSchema.toConfig()` * Remove `.prettierrc`
* Add Martijn Walraven to "Definitions by" section of the package header * Add `validateSDL` function * Add `toConfig` methods See graphql/graphql-js#1331 * Add missing typing for `print` function See graphql/graphql-js#1702 * Rename `MaybePromise` to `PromiseOrValue` graphql/graphql-js#1798 * Rename `blockStringValue.js` to `blockString.js`, and `blockStringValue` to `dedentBlockStringValue` See graphql/graphql-js@16afd2e and graphql/graphql-js@60a2bf8 * Update `graphql` version to 14.2 * Add `.prettierrc` and reformat all files This adds a `.prettierrc` based on the defaults used in DefinitelyTyped#24552 (comment), and reformats all files (only a few are affected). * Fix `GraphQLSchema.toConfig()` * Remove `.prettierrc`
Suggested by @martijnwalraven
MaybePromise
name confuses users into thinking:execute
/graphql
returns either Promise or null/undefined.Also in our codebase, we frequently use
[Mm]aybe*
to represent optional things,e.g.:
graphql-js/src/language/visitor.js
Line 353 in 8c96dc8
graphql-js/src/language/printer.js
Line 244 in 8c96dc8