-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Minor improvement Chelonia #1618
Minor improvement Chelonia #1618
Conversation
…ctually-published
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.
@taoeffect, need your review.
shared/domains/chelonia/internals.js
Outdated
const events = await sbp('chelonia/out/eventsAfter', contractID, recent || contractID) | ||
// checks if the list of events consist of latest event | ||
// TODO: if we use findLastIndex, it will be more clean | ||
// but it needs upgrade Cypress version to 9.7.0 which is of bad performance | ||
// https://docs.cypress.io/guides/references/changelog#9-7-0 | ||
// https://github.com/cypress-io/cypress/issues/22868 | ||
let isLatestExistance = false | ||
for (let i = events.length - 1; i >= 0; i--) { | ||
if (GIMessage.deserialize(events[i]).hash() === latest) { | ||
isLatestExistance = true | ||
break | ||
} | ||
} | ||
if (!isLatestExistance) { | ||
throw new ChelErrorUnexpected() | ||
} |
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.
@taoeffect, not sure if we should use findLastIndex
here, and make it much smart.
Regarding the browser compatibility of findLastIndex
, we should upgrade Cypress version to 9.7.0 or use the Chrome (not Electron) as the cypress default browser in the current version 7.7.0.
I would like to hear your opinion on 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.
References
findLastIndex
Browser Compatibility
- Cypress v7.7.0 browser versions:
Electron: Chromium 94.0.4606.81, Chrome: 113, Firefox: 113
- Cypress v9.7.0 Change Log and Browser versions:
https://docs.cypress.io/guides/references/changelog#9-7-0
Electron: Chromium 100.0.4896.75 - Cypress Bad Performance issue
Cypress is running really slow after upgrading to v10 cypress-io/cypress#22868
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.
Great work @Silver-IT! Nicely done! 👍 Minor changes requested.
shared/domains/chelonia/internals.js
Outdated
} | ||
} | ||
if (!isLatestExist) { | ||
throw new ChelErrorUnexpected() |
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.
throw new ChelErrorUnexpected() | |
throw new ChelErrorUnrecoverable(`expected hash ${latest} in list of events for contract ${contractID}`) |
shared/domains/chelonia/internals.js
Outdated
// but it needs upgrade Cypress version to 9.7.0 which is of bad performance | ||
// https://docs.cypress.io/guides/references/changelog#9-7-0 | ||
// https://github.com/cypress-io/cypress/issues/22868 | ||
let isLatestExist = false |
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.
let isLatestExist = false | |
let latestHashFound = false |
shared/domains/chelonia/internals.js
Outdated
// TODO: if we use findLastIndex, it will be more clean | ||
// but it needs upgrade Cypress version to 9.7.0 which is of bad performance | ||
// https://docs.cypress.io/guides/references/changelog#9-7-0 | ||
// https://github.com/cypress-io/cypress/issues/22868 |
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.
// TODO: if we use findLastIndex, it will be more clean | |
// but it needs upgrade Cypress version to 9.7.0 which is of bad performance | |
// https://docs.cypress.io/guides/references/changelog#9-7-0 | |
// https://github.com/cypress-io/cypress/issues/22868 | |
// TODO: using findLastIndex, it will be more clean but it needs Cypress 9.7+ which has bad performance | |
// https://docs.cypress.io/guides/references/changelog#9-7-0 | |
// https://github.com/cypress-io/cypress/issues/22868 |
shared/domains/chelonia/internals.js
Outdated
@@ -253,7 +253,22 @@ export default (sbp('sbp/selectors/register', { | |||
if (latest !== recent) { | |||
console.debug(`[chelonia] Synchronizing Contract ${contractID}: our recent was ${recent || 'undefined'} but the latest is ${latest}`) | |||
// TODO: fetch events from localStorage instead of server if we have them | |||
const events = await sbp('chelonia/out/eventsSince', contractID, recent || contractID) | |||
const events = await sbp('chelonia/out/eventsAfter', contractID, recent || contractID) | |||
// checks if the list of events consist of latest event |
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.
// checks if the list of events consist of latest event | |
// Sanity check: verify event with latest hash exists in list of events |
hooks && hooks.postpublish && hooks.postpublish(message) | ||
hooks?.prepublish?.(message) | ||
message = await sbp('chelonia/private/out/publishEvent', message, publishOptions) | ||
hooks?.postpublish?.(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.
Nice, I love this simplification of using ?.
and how you added it to many places in the code that needs it!
config.postOp?.(message, state) | ||
config[`postOp_${opT}`]?.(message, state) |
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.
Beautiful 👍
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.
Great job @Silver-IT!! 😄 🎉
Summary of Changes