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

Add entireFieldWrapperValue configuration option, to wrap arrays. #5753

Merged
merged 5 commits into from
Apr 19, 2021

Conversation

hampusborgos
Copy link
Contributor

@hampusborgos hampusborgos commented Mar 26, 2021

Description

Fixes regression introduced in: #3938, fixes feature requested in: #5227 and #4614 (issue can be discussed ether in these issues, or here).

The problem

The change made in issue #3938 changed how fieldWrapper worked, so that it is no longer possible to wrap an entire array using FieldWrapper, rather the values INSIDE the array are wrapped.

@nebbles summed this up well in #5227:

When adding to config:

wrapFieldDefinitions: true
fieldWrapperValue: T | Promise<T> | (() => T) | (() => Promise<T>)

Then I'd expect this wrapper to be applied to the Type of the field

A type like this:

images: Array<GqlImageData>;

Should turn into:

 images: FieldWrapper<Array<GqlImageData>>;

However as mentioned, the above change (#3938) means that Arrays cannot be given a custom wrapper. The result of using the config returns

images: Array<FieldWrapper<GqlImageData>>;

The fix

This adds two new configuration options: wrapEntireFieldDefinitions and entireFieldWrapperValue that fixes this. So when adding to config:

wrapEntireFieldDefinitions: true
entireFieldWrapperValue: T | Promise<T> | (() => T) | (() => Promise<T>)

A type like this:

images: Array<GqlImageData>;

Turns into:

 images: EntireFieldWrapper<Array<GqlImageData>>;

This can also be nested with wrapFieldDefinitions so that

wrapFieldDefinitions: true
fieldWrapperValue: T | Promise<T> | (() => T) | (() => Promise<T>)
wrapEntireFieldDefinitions: true
entireFieldWrapperValue: T | Promise<T> | (() => T) | (() => Promise<T>)

Will make it so a type like this:

images: Array<GqlImageData>;

Turns into:

 images: EntireFieldWrapper<Array<FieldWrapper<GqlImageData>>>;

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Added a new test case for these new configuration option(s).

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

It can also be considered, that this should be added to flow generator also. But I'm not an expert there, so requires assistance.

@changeset-bot
Copy link

changeset-bot bot commented Mar 26, 2021

🦋 Changeset detected

Latest commit: e2b5129

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@graphql-codegen/visitor-plugin-common Minor
@graphql-codegen/typescript Minor
@graphql-codegen/flow Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@hampusborgos hampusborgos changed the title Add entireFieldWrapper configuration option, to wrap arrays. Add entireFieldWrapperValue configuration option, to wrap arrays. Mar 26, 2021
@ibratoev
Copy link

Looks like a good configurable approach with good defaults. I would use that. Thanks! 👍

@dotansimha
Copy link
Owner

Thank you @hampusborgos , this seems like a good direction. Merging and will release soon.

@dotansimha dotansimha merged commit f0b5ea5 into dotansimha:master Apr 19, 2021
@hampusborgos hampusborgos deleted the wrap-entire-fields branch April 26, 2021 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants