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

fix(@nestjs/graphql): support intersection-type for arrays #1420

Closed
wants to merge 2 commits into from
Closed

fix(@nestjs/graphql): support intersection-type for arrays #1420

wants to merge 2 commits into from

Conversation

notmedia
Copy link

@notmedia notmedia commented Mar 5, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

The problem was in createWrappedExplicitTypeFn function, when it executes in reflectTypeFromMetadata the options did not updated, because options modifying in returned callback from createWrappedExplicitTypeFn. But this options need to know before callback executes.

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

#1376

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

}
return explicitTypeRef;
};
let explicitTypeRef = explicitTypeFn();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I remember, this line must be called inside () => (lazily) to prevent circular-dependency-related issues.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then the solution would be to put the options variable in callback like get typeFn works. In this callback I can calculate options, like array depth etc. But as I see this will require a lot of rework. Can you advise better solution? Or I can start to do my variant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kamilmysliwiec I really need this fix ASAP, can you answer me?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kamilmysliwiec so, are you interested to fix this bug?

@juansb827
Copy link

This is still happening 😞

@notmedia
Copy link
Author

@juansb827 Unfortunately the maintainers are not interested to fix this bug 😒

@nestjs nestjs locked as spam and limited conversation to collaborators May 31, 2021
@kamilmysliwiec
Copy link
Member

I'll look into this as soon as I can. Let me lock this for now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants