-
-
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
Chelonia in SW #2357
Conversation
45eccd8
to
9b89d48
Compare
Tests currently passing:
For the non-passing tests, a common theme seems to be that Other things that are broken, but which should be easier to fix:
|
a9068b1
to
4601d99
Compare
068c8c1
to
ba57fb4
Compare
group-income Run #3537
Run Properties:
|
Project |
group-income
|
Branch Review |
feature/chelonia-in-service-worker
|
Run status |
Passed #3537
|
Run duration | 11m 13s |
Commit |
d56c96b5bb ℹ️: Merge 63023d73071255fd0d1c3a65e392b0a4ae890ebc into 5b2b660584d0b93b156bc5fd9927...
|
Committer | Ricardo Iván Vieitez Parra |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
10
|
Skipped |
0
|
Passing |
111
|
View all changes introduced in this branch ↗︎ |
isDMOrMention: isMentionedMe || state.attributes?.type === CHATROOM_TYPES.DIRECT_MESSAGE, | ||
messageType: !newMessage ? MESSAGE_TYPES.TEXT : data.type, | ||
memberID: innerSigningContractID, | ||
chatRoomName: getters.chatRoomAttributes.name | ||
chatRoomName: state.attributes?.name |
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
and currentChatroomId
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 of chelonia.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.
the chatroom that corresponds to this state
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:
chatRoomAttributes (state, getters) {
return getters.currentChatRoomState.attributes || {}
},
Sure, in this instance state
corresponds to getters.currentChatRoomState
, but I don't see how generically we can know what getters.currentChatRoomState
should be (generically). In the app, it works because (remember this is a global getter) there is a currentChatRoomId
. In contracts it works, because currentChatRoomState
can be defined to return 'state'. In the SW, the situation is analogous to the app, but there's no currentChatRoomId
.
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:
const getters = sbp('chelonia/contract/getters', 'gi.contracts/chatroom', state)
getters.currentChatRoomState
But honestly, for simple accessors such as these I don't see the benefit of adding this complexity. This also adds two other factors:
- If the getters are hardcoded (as in, statically imported), it increases bundle size
- If the getters are dynamic, they'll probably need to be async (in case the contract isn't loaded) and because of sandboxing (maybe).
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?
I also think such getters that are simply accessors should be avoided, as they increase code line count and complexity for no benefit.
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.
const getters = sbp('chelonia/contract/getters', 'gi.contracts/chatroom', state)
Yeah, that's a great idea!
I'd make one slight modification: instead of passing in state
, passing in contractID
:
const getters = sbp('chelonia/contract/getters', contractID)
(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.
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.
Since this no longer seems to be needed in the context of this PR (we're importing getters in the SW as needed), can we make this a new issue, possibly linked to the sandboxing issue (#2364)?
8167a3f
to
e5f8ed2
Compare
60603b0
to
286570a
Compare
b4579d2
to
e6a31ed
Compare
e6a31ed
to
17f3e77
Compare
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.
Review ready!
Amazing work @corrideat! This PR represents a lot of hard work and thought. Thank you for your patience as I worked my way through it!
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.
OK - now I think the "final review" 🤞 is ready.
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.
Contender for the Greatest PR in the History of this Project! 😄 🎉
Incredible work @corrideat!!
@taoeffect adds: we should add
VAPID_EMAIL
now to our host server