Skip to content
This repository was archived by the owner on Nov 25, 2021. It is now read-only.

Conversation

@ijsnow
Copy link
Contributor

@ijsnow ijsnow commented Jul 30, 2018

DON'T MERGE

This is a WIP PR to support async ContextResolvers. This should work but is not tested. I stopped working on it because the types seem to be messed up and I have no idea why. I'll Comment on the lines where the issues are.

@ijsnow ijsnow requested a review from felixfbecker July 30, 2018 18:59
return of({ position: { ...context, ...position }, ...rest })
}

return of({ position: undefined, ...rest })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like this type is never considered. If you hover over position on line 379 in this file, you'll see that the type doesn't include | undefined any more.

@codecov
Copy link

codecov bot commented Jul 30, 2018

Codecov Report

Merging #28 into master will decrease coverage by 0.03%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   63.71%   63.68%   -0.04%     
==========================================
  Files          15       15              
  Lines         565      570       +5     
  Branches      146      147       +1     
==========================================
+ Hits          360      363       +3     
- Misses        205      207       +2
Impacted Files Coverage Δ
src/hoverifier.ts 56.49% <71.42%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18098c3...6c58d89. Read the comment docs.

@felixfbecker
Copy link
Contributor

I am more inclined to make a breaking change and require Observable to be returned, for simplicity

@lguychard lguychard closed this Dec 9, 2019
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.

4 participants