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

Updates #93

Merged
merged 35 commits into from
Oct 27, 2020
Merged

Updates #93

merged 35 commits into from
Oct 27, 2020

Conversation

martinheidegger
Copy link
Member

@martinheidegger martinheidegger commented Aug 31, 2020

As part of a security refactoring done with @consento-org/notifications-server this PR updates a whole lot of dependencies. Including the latest version of expo and an updated, more stable notification system.

This is a breaking update that will effectively hide the vaults from users from the current app. Users of the current App that have essential data in their system will be able to simply start an older version (through npm or expo links) to download current data in the vaults, though all relations will be lost.

Task list
  • Use the new notification server
    • Test connection resets
  • Update to expo@3839
    • fix warning about missing splash screen
  • Use get-random-values-polypony
  • Use react-navigation@5 (in progress)
  • Use typescript strict mode (in progress)
  • Fix problem with state user interface that prevents saving of changes
  • Use VirtualizedList for the vault grid
  • Update to latest expo-export (should be minor turned out to be major with the changes in 4 & 5)
    • Vaults
    • Vault
      • FileList
      • Locks
      • Log
      • Fix unlocking
    • Config
      • Fix wrong "updating" display
    • Text Editor
    • Image Editor
    • Consentos
    • Relations
    • Relation
    • New Relation

This is a squashed version of a longer process for consento - version 2.

Big changes summarized:

- It uses the new expo-export @ 5.0.1 that restructured the whole expo output, requiring to touch pretty much every view. See more here: https://github.com/consento-org/expo-export/releases/tag/v4.0.0
- It uses the new react-navigation @ 5 which also restructured the whole setup, requiring to touch pretty much all navigation logic.
- It upgrades to the latest expo@39 version and some features of it. Most notably the new Notification support.
- As part of the notification update, changes and fixes had to be made in the api/crypto/notification-server module which have been updated as well
- To line up with other tooling it uses typescripts strict mode that requires several smaller changes to compile.
- Its a start to comply with several best practices of react-native, such as using StylesSheets, property forwarding, Virtual and typescript 4 features - though there is still room for improvments.

For a history on the creation of this commit: look at https://github.com/consento-org/mobile/tree/history/v2

Signed-off-by: Martin Heidegger <[email protected]>
@martinheidegger martinheidegger marked this pull request as ready for review October 13, 2020 15:19
@martinheidegger
Copy link
Member Author

martinheidegger commented Oct 13, 2020

Having tested it to a reasonable degree for a few days and now released all parts of it (including the notification-server). The PR now contains only one (squashed) commit 6ce40bb that explains the history a bit.

I am hoping for @dkastl or @RangerMauve to review it for general functionality (has it broken important things) and does it still works. Future refactorings in the model layer and stability work on the notification system will need to be postponed to future releases. My open is that the PR can be merged after asserting if it basically works and doesn't break too much.

Copy link
Member

@dkastl dkastl left a comment

Choose a reason for hiding this comment

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

... based on human trust ;-)

@martinheidegger martinheidegger marked this pull request as draft October 16, 2020 03:09
@dkastl
Copy link
Member

dkastl commented Oct 19, 2020

Is this a known issue? #98

@dkastl
Copy link
Member

dkastl commented Oct 19, 2020

Good idea to use newer version of expo-cli as in #100 ?

@martinheidegger
Copy link
Member Author

martinheidegger commented Oct 22, 2020

@dkastl the current version should have mostly updated dependencies (some still outstanding for they might have unintended consequences). Also, as visible in the changelog, many, many bugs and improved a display of error messages. Please run npm start -- --clear to make sure that the latest node_modules are used before a test/review.

Note: the expo builds are broken for the moment, due to updates in how the screenshots are taken. Will update the expo build scripts tomorrow.

@martinheidegger martinheidegger marked this pull request as ready for review October 22, 2020 18:46
@dkastl
Copy link
Member

dkastl commented Oct 23, 2020

What is the goal of this pull request?
It probably makes sense to get it merged at some point even if it isn't perfect.

I tested it with two devices and this worked:

  • start the app
  • create relations
  • create a vault on each
  • add relation as lockee
  • add image to vault
  • lock the vault
  • unlock the vault

However, there seem to be issues, like right after locking the vault an unlock request is sent again.
But maybe we are going to change things anyway, so let's not try to wait until everything is perfect.

@martinheidegger
Copy link
Member Author

martinheidegger commented Oct 23, 2020

What is the goal of this pull request?

The goal is to have a reasonably working mobile app (again) that can be shown to eventual users that uses updated libraries and metaphors (i.e. new expo). The goal is not zero bugs, though it would be good to not have old features broken if possible.

However, there seem to be issues, like right after locking the vault an unlock request is sent again.

Can you reproduce this?

@dkastl
Copy link
Member

dkastl commented Oct 23, 2020

Can you reproduce this?

Probably, but I had to start from beginning with some sort of checklist ... and I need a bit of time. If you find it important to be solved with this PR, then I will describe it in another issue.

@martinheidegger
Copy link
Member Author

I managed to get the same error once by accident but wasn't able to get it consistently which suggest that it may have happened before as well without us noticing. However: if it happens somehow consistently then its likely a new bug that has been introduced and probably needs some looking after. When I have the expo ci running and you think the app is good enough to show in case someone is interested than that state is fine with me. (it definitely wasn't in that state a week ago).

@dkastl
Copy link
Member

dkastl commented Oct 23, 2020

Yes, I'm not sure either, if it's something new or if it has happened already before.
Will test tonight in case INPHO summit is as exciting as last night ;-)

Signed-off-by: Martin Heidegger <[email protected]>
@martinheidegger
Copy link
Member Author

So the last PR finishes the one bug found in #102 merging this PR for now, lets look forward.

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.

2 participants