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

Support subscriptions of value types #3876

Merged
merged 2 commits into from
Jan 2, 2024
Merged

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Jan 2, 2024

The hard part was updating all the tests...

@Shane32 Shane32 added the enhancement New feature or request label Jan 2, 2024
@Shane32 Shane32 requested a review from gao-artur January 2, 2024 12:23
@Shane32 Shane32 self-assigned this Jan 2, 2024
@@ -30,6 +30,20 @@ public async Task Basic()
Observer.ShouldHaveNoMoreResults();
}

[Fact]
public async Task BasicInt()
Copy link
Member Author

Choose a reason for hiding this comment

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

This tests the auto-registering types and worked without changes.

@@ -145,7 +153,7 @@ public MessageInputType()
{
Field<StringGraphType>("fromId");
Field<StringGraphType>("content");
Field<DateGraphType>("sentAt");
Field<DateTimeOffsetGraphType>("sentAt");
Copy link
Member Author

Choose a reason for hiding this comment

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

So the serialized responses would be repeatable as they would carry timezone info which can be set to UTC.

}

[Fact]
public async Task SubscribeInt()
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests in this case direct usage of SourceStreamResolver, which is what all the field builders use.

{
Name = "messageCounter",
Type = typeof(IntGraphType),
StreamResolver = new SourceStreamResolver<int>(context => Subscribe(context).Select(_ => ++counter))
Copy link
Member Author

Choose a reason for hiding this comment

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

Threw exception here previously

Comment on lines 18 to +25
if (typeof(TReturnType).IsValueType)
throw new InvalidOperationException("The generic type TReturnType must not be a value type.");

_sourceStreamResolver = context => new ValueTask<IObservable<object?>>((IObservable<object?>)sourceStreamResolver(context));
{
_sourceStreamResolver = context => new(new ObservableAdapter<TReturnType?>(sourceStreamResolver(context)));
}
else
{
_sourceStreamResolver = context => new((IObservable<object?>)sourceStreamResolver(context));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes in this file are the only "real" changes in the project

Comment on lines +78 to +85
if (typeof(TReturnType).IsValueType)
{
_sourceStreamResolver = async context => new ObservableAdapter<TReturnType?>(await sourceStreamResolver(context.As<TSourceType>()).ConfigureAwait(false));
}
else
{
_sourceStreamResolver = async context => (IObservable<object?>)await sourceStreamResolver(context.As<TSourceType>()).ConfigureAwait(false);
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity Note

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
@codecov-commenter
Copy link

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (b9a113e) 84.66% compared to head (775577f) 84.64%.

Files Patch % Lines
src/GraphQL/Resolvers/SourceStreamResolver.cs 48.00% 13 Missing ⚠️
src/GraphQL/Resolvers/ObservableAdapter.cs 91.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3876      +/-   ##
==========================================
- Coverage   84.66%   84.64%   -0.02%     
==========================================
  Files         419      420       +1     
  Lines       19344    19360      +16     
  Branches     3037     3037              
==========================================
+ Hits        16377    16388      +11     
- Misses       2253     2259       +6     
+ Partials      714      713       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@gao-artur gao-artur left a comment

Choose a reason for hiding this comment

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

Who needs to sleep when you have an OSS to maintain?
That one was fast.

@Shane32 Shane32 linked an issue Jan 2, 2024 that may be closed by this pull request
@Shane32
Copy link
Member Author

Shane32 commented Jan 2, 2024

Who needs to sleep when you have an OSS to maintain? That one was fast.

Yeah, well, my 1yo daughter has a fever and I needed to hold her anyway. I'll be really tired tomorrow.

@Shane32 Shane32 merged commit 3bb4e91 into master Jan 2, 2024
10 checks passed
@Shane32 Shane32 deleted the support_valuetype_subscriptions branch January 2, 2024 12:49
@Shane32 Shane32 added this to the 7.8 milestone Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscription field builders should have 'where T : class'
3 participants