[pull] main from graphql:main #127
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![pull[bot]](https://avatars.githubusercontent.com/in/12910?s=60&v=4){kind=small}
return newPending alongside incremental results when completing a fragment = removes need to track `newPending` and `newIncrementalDataRecords` as part of the class = unifies handling of retrieving new pending for initial result, completed fragments, and stream items incremental entries
Use a stream that never resolves to better demonstrate how there will be a definite pending promise. Calling return on the underlying graph flushes the nextQueue anyway.
Reduces the number of times we need to check as to whether to flush the next queue.
-- we do not need a formal iterator as we are the only consumer.
Currently, when early execution is disabled, we still use the early execution logic to initiate execution groups, which may cause early initiation of non-pending execution groups. alternative to #4109, causes potentially stacking delays when combinations of shared and non-shared keys between sibling defers cause separate deferred grouped field sets for the same deferred fragment.
main improvement is to remove additional intermediate variables, i.e. `map`, `inOriginalResult`.
DeferredGroupedFieldSet => ExecutionGroup SubsequentResultRecord => DeliveryGroup ExecuteExecutionGroups => CollectExecutionGroups
currently not actively being used for code quality improvement — if found useful, can be restored
Currently it is possible to have unhandled promise rejections that arise from a mix of sync and async isTypeOf checks. This should fix that issue.
improves readability at the cost of pushing and then immediate popping
Previously, this was inlined into both executeOperation and collectAndExecuteSubfields, now it is a separate function. executeExecutionPlan calls "getNewDeferMap" instead of "addNewDeferredFragments" to also conform to the spec functional/immutable style. 
…4177) extracted from #3808 PR #3808 uses schema coordinates to improve GraphQL-JS error messages. To reduce the size of the PR, this commit standardizes error messages according to the general pattern that will be introduced with schema coordinates without introducing the coordinates themselves, in the hopes of aiding review of the later PR. EDITED 8/26/2024: I was able to reproduce all of the standardized error messages from #3808 except for the ones in getArgumentValues when it is passed a Field Definition, because the parent type is not passed. Everything else can be calculated for the error messages we are currently printing, although schema coordinates simplifies things. Extracting these changes out of #3808 and rebasing #3808 on main will therefore will better demonstrate how schema coordinates improves the clarity of some of our error messages (namely, getArgumentValues) and simplifies printing them.
This is a rebase of #3847 This implements execution of Fragment Arguments, and more specifically visiting, parsing and printing of fragment-spreads with arguments and fragment definitions with variables, as described by the spec changes in graphql/graphql-spec#1081. There are a few amendments in terms of execution and keying the fragment-spreads, these are reflected in mjmahone/graphql-spec#3 The purpose is to be able to independently review all the moving parts, the stacked PR's will contain mentions of open feedback that was present at the time. - [execution changes](JoviDeCroock#2) - [TypeInfo & validation changes](JoviDeCroock#4) - [validation changes in isolation](JoviDeCroock#5) CC @mjmahone the original author --------- Co-authored-by: mjmahone <[email protected]> Co-authored-by: Yaacov Rydzinski <[email protected]>
To simply suggest a larger cohort of reviewers for more complex changes, and add the caveat that merged changes may be reverted if consensus has not been reached at a GraphQL-JS WG meeting.
#4022) As surfaced in [Discord](https://discord.com/channels/625400653321076807/862957336082645006/1206980831915282532) this currently is a breaking change in the 16.x.x release line which is preventing folks from upgrading towards a security fix. This PR should result in a patch release on the 16 release line. This change was originally introduced to support CFW and browser environments which should still be supported with the `typeof` check CC @n1ru4l This also adds a check whether `.env` is present as in the DOM using `id="process"` defines that as a global which we don't want to access on accident. as shown in #4017 Bundles also target `process.env.NODE_ENV` specifically which fails when it replaces `globalThis.process.env.NODE_ENV` as this becomes `globalThis."production"` which is invalid syntax. Fixes #3978 Fixes #3918 Fixes #3928 Fixes #3758 Fixes #3934 This purposefully does not account for #3925 as we can't address this without breaking CF/plain browsers so the small byte-size increase will be expected for bundled browser environments. As a middle ground we did optimise the performance here. We can revisit this for v17. Most bundlers will be able to tree-shake this with a little help, in #4075 (comment) you can find a conclusion with a repo where we discuss a few. - Next.JS by default replaces [`process.env.NODE_ENV`](https://github.com/vercel/next.js/blob/b0ab0fe85fe8c93792051b058e060724ff373cc2/packages/next/webpack.config.js#L182) you can add `typeof process` linearly - Vite allows you to specify [`config.define`](https://vitejs.dev/config/shared-options.html#define) - ESBuild by default will replace `process.env.NODE_ENV` but does not support replacing `typeof process` - Rollup has a plugin for this https://www.npmjs.com/package/@rollup/plugin-replace Supersedes #4021 Supersedes #4019 Supersedes #3927 > This now also adds a documentation page on how to remove all of these
Co-authored-by: Saihajpreet Singh <[email protected]>
Errata: Left typescript at v5.4.x as deep dependency typedoc requires v5.4.x. Left eslint v8.x as not all plugins are compatible with v9. Code changes, in no particular order: 1. Prettier formatting changes. 2. Prettier moved to an async API, but the `writeGeneratedFile` utility, which previously included prettifying was passed as a callback function to TS and had to stay sync, so prettifying was separated into a separate async function -- the callback function luckily did not seem to actually requiring another round of prettifying, as it just involved renaming. All the other callsites of the new utility had to be made async. In the alternate, I investigated @prettier/sync and the lower-level make-synchronized and make-synchronous packages, but I could not get them working. 3. Plenty of eslint rule changes! I have tried to make sure that the rule list orders now match the linked documentation, so that further updates might be easier. 4. Minor docusaurus config tweaks to get the build to pass.
[#3086 rebased on main](#3086). Depends on #3812 @leebyron comments from original PR: > Factors out input validation to reusable functions: > > * Introduces `validateInputLiteral` by extracting this behavior from `ValuesOfCorrectTypeRule`. > * Introduces `validateInputValue` by extracting this behavior from `coerceInputValue` > * Simplifies `coerceInputValue` to return early on validation error > * Unifies error reporting between `validateInputValue` and `validateInputLiteral`, causing some error message strings to change, but error data (eg locations) are preserved. > > These two parallel functions will be used to validate default values > > Potentially breaking if you rely on the existing behavior of `coerceInputValue` to call a callback function, as the call signature has changed. GraphQL behavior should not change, though error messages are now slightly different. Note: also breaking if you rely on the default callback function to throw. Grossly similar behavior is available with `validateInputValue()`. Co-authored-by: Lee Byron <[email protected]>
Alternative to #2181 This adds a new `findSchemaChanges` export and deprecates the breaking and dangerous alternative to be removed in v18. While adding safe-schema-changes I noticed that we lack directive arg validation here, we should probably add these to breaking/dangerous changes in the future.
This adds support for aborting execution from the outside or resolvers, this adds a few tests and tries to make the support as easy as possible. Do we want to support having abort support on subscriptions, I guess it makes sense for server-sent events. I've chosen 2 places to place these interrupts - `executeFieldsSerially` - every time we start a new mutation we check whether the runtime has interrupted - `executeFields` - every time we start executing a new field we check whether the runtime has interrupted - inside of the catch block as well so we return a singular error, all though this doesn't really matter as the consumer would not receive anything - this here should also take care of deferred fields When comparing this to `graphql-tools/execute` I am not sure whether we want to match this behavior, this throws a DomException which would be a whole new exception that gets thrown while normally during execution we wrap everything with GraphQLErrors. Supersedes #3791 Resolves #3764 Co-authored-by: yaacovCR <[email protected]>
This way people can add this to metrics and inform themselves about sane values to use with the `maxTokens` option on `parse`. This was added as a non-enumerable property to increase backwards compatability. Currently there is now way to know the tokens and by extension set sane defaults.
incremental completion errors are indicated if and only if null bubbling has caused a previously sent response to be null we must instead just give a partial response with errors within the incremental array The GraphQL Tools implementation of this feature has different behavior where the next call will throw if the operation is aborted, but we have opted to give partial responses, and so we must adjust.
[#3049 rebased on main](#3049). This is the last rebased PR from the original PR stack concluding with #3049. * Rebased: #3809 [Original: #3092] * Rebased: #3810 [Original: #3074] * Rebased: #3811 [Original: #3077] * Rebased: #3812 [Original: #3065] * Rebased: #3813 [Original: #3086] * Rebased: #3814 (this PR) [Original: #3049] Update: #3044 and #3145 have been separated from this stack. Changes from original PR: 1. `astFromValue()` is deprecated instead of being removed. @leebyron comments from #3049, the original PR: > Implements [graphql/graphql-spec#793](graphql/graphql-spec#793) > > * BREAKING: Changes default values from being represented as an assumed-coerced "internal input value" to a pre-coerced "external input value" (See chart below). > This allows programmatically provided default values to be represented in the same domain as values sent to the service via variable values, and allows it to have well defined methods for both transforming into a printed GraphQL literal string for introspection / schema printing (via `valueToLiteral()`) or coercing into an "internal input value" for use at runtime (via `coerceInputValue()`) > To support this change in value type, this PR adds two related behavioral changes: > > * Adds coercion of default values from external to internal at runtime (within `coerceInputValue()`) > * Removes `astFromValue()`, replacing it with `valueToLiteral()` for use in introspection and schema printing. `astFromValue()` performed unsafe "uncoercion" to convert an "Internal input value" directly to a "GraphQL Literal AST", where `valueToLiteral()` performs a well defined transform from "External input value" to "GraphQL Literal AST". > * Adds validation of default values during schema validation. > Since assumed-coerced "internal" values may not pass "external" validation (for example, Enum values), an additional validation error has been included which attempts to detect this case and report a strategy for resolution. > > Here's a broad overview of the intended end state: > >  --------- Co-authored-by: Lee Byron <[email protected]>
this allows e.g. passing the signal to fetch Note: the `abortSignal` is now the fifth argument to a GraphQLFieldResolverFn. If no resolver if provided, and the parent is an object with a key for the field name with a value that is a function, the `abortSignal` will be the fourth argument, as in the included test, with the `parent` accessible via the `this` keyword.
for readability. benchmarks suggest similar or even better performance 
This gets rid of enums for two reasons - TypeScript does not really compile them size efficiently, see [here](https://www.runpkg.com/[email protected]/language/kinds.mjs) - Raw size kinds.js before: 2800bytes - Raw size kinds.js after: 2200 bytes - It's not kind to plains strings as seen in #3356 Resolves #3356 This does not intend to fix the tree-shaking issue as seen in #4253, for that we'd need to fully normalize these exports and sacrifice the DX of the namespaces. I do think that it becomes easier as plain strings will be "easier" compared to before so if you're not doing comparisons you can opt out of the `Kind` namespace. <details> <summary>See new output</summary> ```js /** * The set of allowed kind values for AST nodes. */ exports.Kind = { /** Name */ NAME: 'Name', /** Document */ DOCUMENT: 'Document', OPERATION_DEFINITION: 'OperationDefinition', VARIABLE_DEFINITION: 'VariableDefinition', SELECTION_SET: 'SelectionSet', FIELD: 'Field', ARGUMENT: 'Argument', FRAGMENT_ARGUMENT: 'FragmentArgument', /** Fragments */ FRAGMENT_SPREAD: 'FragmentSpread', INLINE_FRAGMENT: 'InlineFragment', FRAGMENT_DEFINITION: 'FragmentDefinition', /** Values */ VARIABLE: 'Variable', INT: 'IntValue', FLOAT: 'FloatValue', STRING: 'StringValue', BOOLEAN: 'BooleanValue', NULL: 'NullValue', ENUM: 'EnumValue', LIST: 'ListValue', OBJECT: 'ObjectValue', OBJECT_FIELD: 'ObjectField', /** Directives */ DIRECTIVE: 'Directive', /** Types */ NAMED_TYPE: 'NamedType', LIST_TYPE: 'ListType', NON_NULL_TYPE: 'NonNullType', /** Type System Definitions */ SCHEMA_DEFINITION: 'SchemaDefinition', OPERATION_TYPE_DEFINITION: 'OperationTypeDefinition', /** Type Definitions */ SCALAR_TYPE_DEFINITION: 'ScalarTypeDefinition', OBJECT_TYPE_DEFINITION: 'ObjectTypeDefinition', FIELD_DEFINITION: 'FieldDefinition', INPUT_VALUE_DEFINITION: 'InputValueDefinition', INTERFACE_TYPE_DEFINITION: 'InterfaceTypeDefinition', UNION_TYPE_DEFINITION: 'UnionTypeDefinition', ENUM_TYPE_DEFINITION: 'EnumTypeDefinition', ENUM_VALUE_DEFINITION: 'EnumValueDefinition', INPUT_OBJECT_TYPE_DEFINITION: 'InputObjectTypeDefinition', /** Directive Definitions */ DIRECTIVE_DEFINITION: 'DirectiveDefinition', /** Type System Extensions */ SCHEMA_EXTENSION: 'SchemaExtension', /** Type Extensions */ SCALAR_TYPE_EXTENSION: 'ScalarTypeExtension', OBJECT_TYPE_EXTENSION: 'ObjectTypeExtension', INTERFACE_TYPE_EXTENSION: 'InterfaceTypeExtension', UNION_TYPE_EXTENSION: 'UnionTypeExtension', ENUM_TYPE_EXTENSION: 'EnumTypeExtension', INPUT_OBJECT_TYPE_EXTENSION: 'InputObjectTypeExtension', }; ``` </details> Currently seeing whether we should disable the redeclare lint-rule and whether there is a better way to type our `nodeKinds`
…lvers (#4267) Prior to this pull request, cancellation worked by checking the abort signal status during execution, and throwing the reason if the abort signal has been triggered. This fails if an asynchronous resolver hangs. This pull request changes the cancellation method to wrap promises returned by resolvers so that they immediately reject on cancellation.
moves simplest case inside the execute[Root|Sub]ExecutionPlan() functions to better match proposed spec edits orders function in call order, moving executeSubExecutionPlan down closer to where it is used no significant change in performance 
This adds a new safe schema-change when a description changes of any schema-member. Do note that there are some drive-by fixes in here as I noticed a bug in safe-argument changes where we'd just always report the changes rather than actually see whether something changed 😅 Resolves #4245
Resolves #4237 This mirrors the fields/input-object argument logic but specifically for directives.
When the abort signal is triggered, any pending `.next()` calls should return immediately
we must update the lexer line number and the line start position, because lookahead saves the token within the linked list, and so will never be called again on this token we do not change the current token, however, until the lexer is advanced closes #2764
- to separate between this repo and the spec - suggest adding PRs with failing tests when reporting bugs - add more flexibility to review/merge process - suggest using discord to communicate
motivation: 1. can be used to extract common logic from extendSchemaImpl and lexicographicSortSchema 2. can be used further enhance extendSchemaImpl to take resolvers 3. can be exposed to provide a generic safe mapSchemaConfig
…QLEnumValue (#4288) this extracts logic from #3044 and #3145 (later rebased as #3807 and #3808) to implement more informative error messages without implementing [the full schema coordinate RFC](graphql/graphql-spec#794) This is a BREAKING CHANGE because these schema elements are now longer plain objects and function differently in various scenarios, for example with `String(<schemaElement>` `JSON.stringifu(<schemaElement>` and `.toString()` and `.toJSON()` --------- Co-authored-by: Jovi De Croock <[email protected]>
I was just trying to understand the new flows for how scalar values are represented/parsed/serialized, and noticed that the comments for a couple of these methods looked backwards. Really hoping I am not just confused about the terminology, and that these aren't actually correct 😅
New Spec PR: graphql/graphql-spec#1098 Old Spec PR: graphql/graphql-spec#947 Original issue raised by @benjaminjkraft : #3592
this way, if (when?) we re-enable incemental delivery support with subscriptions, the signature of collect fields will not need to change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot]
Can you help keep this open source service alive? 💖 Please sponsor : )