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 useService hook and modern Context #242

Merged
merged 11 commits into from
Apr 10, 2019
Merged

Conversation

micburks
Copy link
Contributor

@micburks micburks commented Apr 1, 2019

RFC

  • Implement modern Context API for services
  • Implement useService hook
  • add tests

@codecov
Copy link

codecov bot commented Apr 1, 2019

Codecov Report

Merging #242 into master will increase coverage by 2.19%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #242      +/-   ##
========================================
+ Coverage    74.8%    77%   +2.19%     
========================================
  Files          13     14       +1     
  Lines         262    287      +25     
  Branches       55     58       +3     
========================================
+ Hits          196    221      +25     
  Misses         49     49              
  Partials       17     17
Impacted Files Coverage Δ
src/index.js 72.41% <100%> (+0.98%) ⬆️
src/context.js 100% <100%> (ø)

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 dd5ba37...47fd8d3. Read the comment docs.

src/__tests__/context.node.js Outdated Show resolved Hide resolved
src/context.js Outdated Show resolved Hide resolved
@ganemone
Copy link
Contributor

ganemone commented Apr 1, 2019

Only thing missing is an update to the readme with documentation on the useService hook and might be good to add a test that documents the behavior when you call useService on a token with no value registered. Does it throw an error or return null? I think we probably want it to throw, but not totally sure. Worth some discussion

src/context.js Outdated Show resolved Hide resolved
@micburks
Copy link
Contributor Author

micburks commented Apr 1, 2019

Only thing missing is an update to the readme with documentation on the useService hook and might be good to add a test that documents the behavior when you call useService on a token with no value registered. Does it throw an error or return null? I think we probably want it to throw, but not totally sure. Worth some discussion

Right now there is no difference between a token that hasn't been registered, a plugin that has no provides, or a plugin that has a provides that returns undefined; they all return undefined. We could throw in all these situations, since it doesn't make sense to use useService if you're not expecting anything to be returned, or we could add another method hasService to the base app in fusion-core so we can have more accurate messaging for errors.

AlexMSmithCA
AlexMSmithCA previously approved these changes Apr 2, 2019
@AlexMSmithCA
Copy link
Member

Cool, changes look great!

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
export const FusionContext = React.createContext<any>({});
export const ServiceContext = React.createContext<any>(() => {});

type ReturnsType<T> = () => T;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed a mechanism to support Token.optional since flow considers it different from Token. This technically allows someone to pass in something that isn't a token. The usage of this function is so straightforward that I don't think this is a huge issue.

@micburks
Copy link
Contributor Author

!merge

@old-fusion-bot old-fusion-bot bot merged commit 0a3eeb4 into master Apr 10, 2019
@old-fusion-bot
Copy link

Triggered Fusion.js build verification: https://buildkite.com/uberopensource/fusion-release-verification/builds/1943

@old-fusion-bot old-fusion-bot bot deleted the use-service-hook branch April 10, 2019 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants