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 dappkit-web functionality #7328

Merged
merged 7 commits into from
Mar 5, 2021
Merged

Add dappkit-web functionality #7328

merged 7 commits into from
Mar 5, 2021

Conversation

eelanagaraj
Copy link
Contributor

@eelanagaraj eelanagaraj commented Mar 3, 2021

Want to get some initial feedback since there was a lot of discussion on how we want to structure the separate modules.

  • In this PR, there are a few functions that now have web-specific counterparts(waitForAccountAuthWeb, waitForSignedTxsWeb, parseURLOnRender).
  • I implemented these in a decorator-like style, so refactored the mobile functions in the process (and haven't published/tested these yet), but we can always remove the majority of the mobile changes if need be.
  • I didn't create counterparts for listenToAccount or listenToSignedTxs -- it seems like we never use these in the tutorials/docs anyways (maybe it makes sense to deprecate these?? the functionality is very very similar to the waitFor* functions except for not requiring a requestId)

(web mods heavily based on Alex's workaround in use-contractkit
(- Includes Alex's change from Expo -> react-native (can rebase again once that is pushed to devx/update-dappkit))

Closes #7348

Copy link
Contributor

@AlexBHarley AlexBHarley left a comment

Choose a reason for hiding this comment

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

Looks good! I think this is a better direction to be taking (separate implementations for web and react-native).

I'd prefer we move the web package into its own file rather than suffixing functions with *Web. That way we could just do import {waitForAccountAuth} from '@celo/dappkit/web.

It also probably makes sense for there to be a timeout on the polling action? If not, at least we should provide a way to cancel polling. Reading LocalStorage every 100ms until someone refreshes their isn't terrible but not ideal either.

@eelanagaraj
Copy link
Contributor Author

eelanagaraj commented Mar 4, 2021

@AlexBHarley split into two files for now but without any special nesting/configuration--was playing around with this a bit but it felt messy to have nested tsconfigs and wasn't working as cleanly as I'd hoped, so for now web imports would look like import {...} from @dappkit/lib/web. Also export all of the same crossover functions in dappkit-web to allow for users to just import one vs. the other (even for shared functions).

Added a simple timeout for now, but yeah I think canceling dappkit flows would be a nice feature (and has a different ticket out for it?) will continue thinking about how that could look!

@eelanagaraj eelanagaraj marked this pull request as ready for review March 5, 2021 09:35
@eelanagaraj eelanagaraj requested review from asaj, cmcewen, mcortesi, timmoreton and a team as code owners March 5, 2021 09:35
Copy link
Contributor

@AlexBHarley AlexBHarley left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@AlexBHarley AlexBHarley added the automerge Have PR merge automatically when checks pass label Mar 5, 2021
@eelanagaraj eelanagaraj changed the base branch from devx/update-dappkit to master March 5, 2021 18:16
@eelanagaraj eelanagaraj force-pushed the eelanagaraj/dappkit-web branch from a43175b to 8c28a71 Compare March 5, 2021 18:31
@mergify mergify bot merged commit ced6624 into master Mar 5, 2021
@mergify mergify bot deleted the eelanagaraj/dappkit-web branch March 5, 2021 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dappkit-web modifications
2 participants