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

createAppConnector(): stop using instanceof #197

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

bpierre
Copy link
Contributor

@bpierre bpierre commented Aug 12, 2020

The app instance passed to createAppConnector() might come from the a version of @aragon/connect-core that is a dependent of @aragon/connect, which is generally included as a direct dependency by the final app.

createAppConnector() itself comes from the app connector package, which might contain a different version of @aragon/connect-core.

These situations make the use of typeof impossible, as two App constructors would come from the two different modules.

There are also other situations where linking one dependency (e.g. the app connector only) can create the same issue.

This commits moves to duck typing to validate the App instance, which also make it work in these different scenarios.

The app instance passed to createAppConnector() might come from the
a version of @aragon/connect-core that is a dependent of
@aragon/connect, which is generally included as a direct dependency by
the final app.

createAppConnector() itself comes from the app connector package, which
might contain a different version of @aragon/connect-core.

These situations make the use of typeof impossible, as two App
constructors would come from the two different modules.

There are also other situations where linking one dependency (e.g. the app
connector only) can create the same issue.

This commits moves to duck typing to validate the App instance, which
also make it work in these different scenarios.
Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

🙌

@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #197 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   34.93%   34.91%   -0.02%     
==========================================
  Files          98       98              
  Lines        1792     1793       +1     
  Branches      274      275       +1     
==========================================
  Hits          626      626              
- Misses       1166     1167       +1     
Flag Coverage Δ
#unittests 34.91% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/connect-core/src/utils/app-connectors.ts 0.00% <0.00%> (ø)

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 628c0e9...e07d898. Read the comment docs.

@bpierre bpierre merged commit 7aa74bd into master Aug 12, 2020
@bpierre bpierre deleted the create-app-connector-no-instanceof branch August 12, 2020 13:19
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.

2 participants