-
-
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
Groundwork to move Chelonia into a service worker #1981
Conversation
d1d6044
to
9639c3b
Compare
9639c3b
to
c1dab49
Compare
Passing run #2213 ↗︎
Details:
Review all test suite changes for PR #1981 ↗︎ |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@sbp/sbp@2.2.0 |
Object.defineProperty(obj, raw, { value: true }) | ||
return obj |
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.
Could you:
- Add a comment explaining why
Object.defineProperty
is used instead ofobj[raw] = true
? - I think this can be shortened to a 1-liner if you put the
return
in front of theObject
on line 20
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.
this can be shortened to a 1-liner if
I don't think that contributes to readability, because it's not self-evident that Object.defineProperty(a, b)
returns a
.
const deserializerTable = Object.create(null) | ||
|
||
// The `deserializer` function reconstructs data on the receiving side | ||
export const deserializer = (data: any) => { |
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 passed in? I see JSON.stringify
is called on data
. Normally a deserializer takes in a serialized value, which in the case of JSON is a string. However, the fact that JSON.stringify
is being used here indicates that this is not a JSON string. If so, then it sounds like data
is the result of calling JSON.parse
elsewhere on the serialized JSON string that was passed over the message port..? Or am I mistaken? Anyway, this all should be clarified and explaining in a comment.
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.
You're maybe getting confused because of the calls to JSON.stringify
and JSON.parse
. Those are incidental and an implementation detail. Both serialize
and deserialize
use JSON.parse(JSON.stringify(data, a), b)
. The 'magic' is in what a
and b
do, and JSON
happens to be used because it traverses objects and is relatively fast.
if (value && value[raw]) return value | ||
if (value === undefined) return rawResult(['_', '_']) | ||
if (!value) return value | ||
if (Array.isArray(value) && value[0] === '_') return rawResult(['_', '_', ...value]) |
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.
Can you add a comment explaining what this is checking for?
// joinChatRoom sideEffect will trigger a call to 'gi.actions/chatroom/join', we want | ||
// to wait for that action to be received and processed, and then switch the UI to the | ||
// new chatroom. We do this here instead of in the sideEffect for chatroom/join to | ||
// avoid causing the UI to change in other open tabs/windows, as per bug: | ||
// https://github.com/okTurtles/group-income/issues/1960 | ||
onprocessed: (msg) => { | ||
const fnEventHandled = (cID, message) => { | ||
if (cID === chatRoomID) { | ||
if (sbp('state/vuex/getters').isJoinedChatRoom(chatRoomID)) { | ||
sbp('state/vuex/commit', 'setCurrentChatRoomId', { chatRoomID, groupID: msg.contractID() }) | ||
sbp('okTurtles.events/off', EVENT_HANDLED, fnEventHandled) | ||
} | ||
} | ||
} | ||
sbp('okTurtles.events/on', EVENT_HANDLED, fnEventHandled) | ||
}, |
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.
Why was this removed?
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.
Because that deals with the Vuex state and doesn't belong into actions.
frontend/controller/app/group.js
Outdated
'gi.app/group/addAndJoinChatRoom': async function (params: GIActionParams) { | ||
const chatRoomID = await sbp('gi.actions/group/addAndJoinChatRoom', params) | ||
await sbp('chelonia/contract/wait', chatRoomID) | ||
// joinChatRoom sideEffect will trigger a call to 'gi.actions/chatroom/join', we want | ||
// to wait for that action to be received and processed, and then switch the UI to the | ||
// new chatroom. We do this here instead of in the sideEffect for chatroom/join to | ||
// avoid causing the UI to change in other open tabs/windows, as per bug: | ||
// https://github.com/okTurtles/group-income/issues/1960 | ||
if (sbp('state/vuex/getters').isJoinedChatRoom(chatRoomID)) { | ||
sbp('state/vuex/commit', 'setCurrentChatRoomId', { chatRoomID, groupID: params.contractID }) | ||
} | ||
|
||
return chatRoomID | ||
} |
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.
Why was this separated out like this?
I see no explanation for the separation of actions
and app
folders.
Could you maybe create a README.md
inside of controller/
that explains 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.
I'm not sure if this is an actual question or just a request to create a README.md
explaining it, but the app
name was your idea. The reason for the separation is that app
runs on the... app (as in, in the browser window context) and actions run in the service worker context.
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.
just a request to create a README.md explaining it
👍
// loading the website instead of stalling out. | ||
/* try { | ||
if (!state) { | ||
// Make sure we don't unsubscribe from our own identity contract | ||
// Note that this should be done _after_ calling | ||
// `chelonia/storeSecretKeys`: If the following line results in | ||
// syncing the identity contract and fetching events, the secret keys | ||
// for processing them will not be available otherwise. | ||
await sbp('chelonia/contract/retain', identityContractID) | ||
} else { | ||
// If there is a state, we've already retained the identity contract | ||
// but might need to fetch the latest events | ||
await sbp('chelonia/contract/sync', identityContractID, { force: true }) | ||
} | ||
} catch (err) { | ||
sbp('okTurtles.events/emit', LOGIN_ERROR, { username, identityContractID, error: err }) | ||
const errMessage = err?.message || String(err) | ||
console.error('Error during login contract sync', errMessage) | ||
|
||
const promptOptions = { | ||
heading: L('Login error'), | ||
question: L('Do you want to log out? Error details: {err}.', { err: err.message }), | ||
primaryButton: L('No'), | ||
secondaryButton: L('Yes') | ||
} | ||
|
||
const result = await sbp('gi.ui/prompt', promptOptions) | ||
if (!result) { | ||
return sbp('gi.app/identity/logout') | ||
} else { | ||
throw err | ||
} | ||
} | ||
*/ |
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.
This was important and commonly used code that comes into play frequently during development (anytime the server is restarted).
Why is it commented out and how will this be handled instead?
EDIT: oh I see, is the code below supposed to replace it?
/* hooks: { | ||
handleEventError: (e: Error, message: GIMessage) => { | ||
if (e.name === 'ChelErrorUnrecoverable') { | ||
sbp('gi.ui/seriousErrorBanner', e) | ||
} | ||
if (sbp('okTurtles.data/get', 'sideEffectError') !== message.hash()) { | ||
// Avoid duplicate notifications for the same message. | ||
errorNotification('handleEvent', e, message) | ||
} | ||
}, | ||
processError: (e: Error, message: GIMessage, msgMeta: { signingKeyId: string, signingContractID: string, innerSigningKeyId: string, innerSigningContractID: string }) => { | ||
if (e.name === 'GIErrorIgnoreAndBan') { | ||
sbp('okTurtles.eventQueue/queueEvent', message.contractID(), [ | ||
'gi.actions/group/autobanUser', message, e, msgMeta | ||
]) | ||
} | ||
// For now, we ignore all missing keys errors | ||
if (e.name === 'ChelErrorDecryptionKeyNotFound') { | ||
return | ||
} | ||
errorNotification('process', e, message) | ||
}, | ||
sideEffectError: (e: Error, message: GIMessage) => { | ||
sbp('gi.ui/seriousErrorBanner', e) | ||
sbp('okTurtles.data/set', 'sideEffectError', message.hash()) | ||
errorNotification('sideEffect', e, message) | ||
} | ||
} */ |
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 are these going to be handled?
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.
This is still in progress and I don't have an answer at this time. The problematic part here is the notifications. Having contracts run in a SW will require re-thinking how notifications work.
'gi.db/settings/save': function (user: string, value: any): Promise<*> { | ||
return appSettings.setItem(user, value) | ||
return appSettings.setItem('u' + user, value) |
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's the reason for this u
prefix? (Can you add a comment?)
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.
It's to disambiguate the type of data that's being stored. I can add a comment.
groupProfile (state, getters) { | ||
return member => { | ||
const profiles = getters.currentGroupState.profiles | ||
return profiles && profiles[member] && { | ||
...profiles[member], | ||
get lastLoggedIn () { | ||
return getters.currentGroupLastLoggedIn[member] || this.joinedDate | ||
} | ||
} | ||
} | ||
}, | ||
groupProfiles (state, getters) { | ||
const profiles = {} | ||
for (const member in (getters.currentGroupState.profiles || {})) { | ||
const profile = getters.groupProfile(member) | ||
if (profile.status === PROFILE_STATUS.ACTIVE) { | ||
profiles[member] = profile | ||
} | ||
} | ||
return profiles | ||
}, | ||
groupCreatedDate (state, getters) { | ||
return getters.groupProfile(getters.currentGroupOwnerID).joinedDate | ||
}, | ||
groupMincomeAmount (state, getters) { | ||
return getters.groupSettings.mincomeAmount | ||
}, | ||
groupMincomeCurrency (state, getters) { | ||
return getters.groupSettings.mincomeCurrency | ||
}, |
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.
As far as I can tell, these and related getters were copied here from the group contract.
- Why?
- Why were they not deleted from group.js? (DRY)
Getters are useful in contracts. If you are moving so many of them out of contracts then we need to come up with a total replacement for them and rewrite a lot of the code in the group.js contract.
Or we need to come up with a way to make them work the way they did before (or in similar fashion) without violating DRY.
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.
As far as I can tell, these and related getters were copied here from the group contract.
Correct.
- Because that seemed like the easiest thing to do with the fewest number of changes required (an alternative would have been to move them to a different file which is then imported from both places).
- Because they're needed there. In particular, after this change, contract getters and root getters are no longer the same and cannot call each other (at least, not synchronously).
Getters are useful in contracts. If you are moving so many of them out of contracts then we need to come up with a total replacement for them and rewrite a lot of the code in the group.js contract.
That's correct. I think we should probably get rid of the concept of getters in contracts altogether, as that's a Vue.js concept anyhow. However, a shorter term alternative is to import them from a different shared file, to avoid source duplication (there'll still be duplication in the build artifacts).
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.
I definitely agree we would need to at the very least fix the DRY issue in the source files.
I think we should probably get rid of the concept of getters in contracts altogether, as that's a Vue.js concept anyhow.
It's a very useful concept for building Vue.js GUIs. We would need to keep it just for that, as otherwise it would require rewriting a bunch of the frontend code as well, and we don't want to do that not just because we don't have time for it, but because we want to support Vue.js and similar libraries that use the concept of getters.
So what is your proposal...?
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.
It's a very useful concept for building Vue.js GUIs
Precisely my point. It's useful for Vue.js. It's not generally useful outside of Vue.js ('it' refers to the 'getters' API, not to the idea of a function that returns a value), because it's not a universal concept (e.g., Redux doesn't have them) and even if it were a common enough concept, it's currently only useful (in the sense of having 'getters' be part of a contract definition) because the API is made to be exactly what Vue.js uses, which may be different for other frameworks.
I'm not saying to get rid of them on Vue.js, Vue.js can still use them. What I'm saying is that Vue.js doesn't have access to them in contracts and so the whole idea that getters can be shared is just not feasible. So, for contracts, there'd be nothing to replace them. However, since contracts can run arbitrary code, they can define their own getters and for applications that use Vue.js, they can use the same getter signature that's used in Vue.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.
To expand on the not feasible part:
I mean that it cannot be literally the same function reference, as it's done now on master
, where sbp('state/vuex/getters').groupCreatedDate
will use some code that's defined in the group contract. This is because the UI is running on the main thread and the group contract is running in a SW (potentially in yet another context after sandboxing). What can be done, as I suggested, is moving the group getters definitions to a different file, and importing that from the app and the contract.
…up-chat-message, group-paying
Closing in favor of #2357 |
No description provided.