-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Chelonia in SW #2357
Open
corrideat
wants to merge
41
commits into
master
Choose a base branch
from
feature/chelonia-in-service-worker
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Chelonia in SW #2357
Changes from 9 commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
9b89d48
Chelonia in SW
corrideat bce3c5c
Debug
corrideat 4601d99
Merge branch 'master' into feature/chelonia-in-service-worker
corrideat 15d5f86
Merge branch 'master' into feature/chelonia-in-service-worker
corrideat 2861605
Test fixes
corrideat 80b1385
Fix group-chat-direct-message spec
corrideat 693173f
Fix issue with the no-results slot being (incorrectly) used
corrideat ba57fb4
Changes supporting failing chat tests
corrideat 13b4357
Fix attachments in SW
corrideat 0a90ec4
Fix Flow fypes
corrideat 22a0f6e
Use atomic for chatroom members
corrideat e5f8ed2
Chat bugfixes
corrideat 286570a
Merge branch 'master' into feature/chelonia-in-service-worker
corrideat ab7d69d
Merge branch 'master' into feature/chelonia-in-service-worker
corrideat 3c78f54
Use session storage for tab logs. Refactor logging to be more generic.
corrideat c8a4441
Bugfixes and removal of unnecessary selectors
corrideat 17f3e77
SW logs
corrideat fe4521a
Bugfix for loading preferences (fix event handlers)
corrideat 870d9a6
State for KV events
corrideat 001e813
Serious banner error
corrideat 48db75e
More consistent chelonia / vuex state use
corrideat d99c1e0
Remove debug logging
corrideat 90460e7
Merge branch 'master' into feature/chelonia-in-service-worker
corrideat 279e171
Logs UI
corrideat 933957b
Merge branch 'master' into feature/chelonia-in-service-worker
corrideat ce94156
Lint
corrideat bdd0f84
Autologout for non-exisiting identity contracts
corrideat 218dd9b
Merge branch 'master' into feature/chelonia-in-service-worker
corrideat aa12e08
Safari workaround
corrideat fd77e0b
Bugfix
corrideat 18046a7
Bugfixes
corrideat bbb7277
Logout flow fixes
corrideat f7bc3d4
Last logged in event
corrideat b08b48b
Avoid SW logs spam
corrideat 0f55f24
Chatroom position events
corrideat 332e8cb
Feedback
corrideat 6fd370f
WIP
corrideat fe4e55d
Stability bugfixes
corrideat ca116a5
Merge branch 'master' into feature/chelonia-in-service-worker
corrideat e4f40bc
Port notifications code
corrideat 74b2dfa
Backend functions for sending
corrideat File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is preventing us from moving the getter definitions into a file that is shared by the frontend and the SW?
That would be much preferable to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatroom getters are a Vuex module, which are harder to emulate without Vuex because of state partitioning. In addition, those getters rely on
currentGroupId
andcurrentChatroomId
which can't be used in contracts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there nothing creative that's possible here? We can't define getters in a way that has them referring to the "the chatroom that corresponds to this
state
"?i.e. "state paritioned getters" ala
gettersProxy
? (see bottom ofchelonia.js
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would that even be possible the way getters are defined? I don't really see an issue with the lines highlighted, and I'm not even sure why we'd need a getter that returns a single property.
But going to the definition:
Sure, in this instance
state
corresponds togetters.currentChatRoomState
, but I don't see how generically we can know whatgetters.currentChatRoomState
should be (generically). In the app, it works because (remember this is a global getter) there is acurrentChatRoomId
. In contracts it works, becausecurrentChatRoomState
can be defined to return 'state'. In the SW, the situation is analogous to the app, but there's nocurrentChatRoomId
.Now, this could maybe work by having a getter that takes the contractID as a function parameter, but this requires refactoring and adds complexity. I also think such getters that are simply accessors should be avoided, as they increase code line count and complexity for no benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, another solution along the lines of what you suggested could be something like:
But honestly, for simple accessors such as these I don't see the benefit of adding this complexity. This also adds two other factors:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corrideat would it be possible to instantiate dynamically a new batch of getters where
getters. currentChatRoomState
returns a specific contract state in the SW using some sort of factory function?Getters are very important, as they solve a lot of DRY related problems. By using getters we can ensure that the way that data is accessed everywhere will have consistent return values (e.g. because it uses tricks like
|| {}
etc.). Whereas those considerations would have to be thought of each time you access state data directly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a great idea!
I'd make one slight modification: instead of passing in
state
, passing incontractID
:(EDIT: I don't think you need to pass in the name, that can be retrieved using
contractID
if needed)Then yeah, at least we could reuse the contract getters that are already defined in the contracts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update based on today's conversation: we could use getters synchronously by importing the contract getters from this actions file. However, it'd have the downside of being 'statically hardcoded' based on whatever version is imported at build time and will increase bundle size. Doing it via Chelonia is potentially possible, but it'd need to be async in case the contract isn't cached and need to be loading and also maybe because of sandboxing.