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

Refactoring and fixes in postback pipeline #885

Merged
merged 32 commits into from Oct 18, 2020

Conversation

tomasherceg
Copy link
Member

This PR contains several things:

  • updates to the postback pipeline and fixes of several race conditions
  • new Jest tests from Migrate JS tests to Jest #883
  • refactoring of all event arguments with tests that make sure the events are fired in the correct order and that they contain all necessary arguments

Tomáš Herceg and others added 27 commits August 30, 2020 15:18
The postbacks are disabled on SPA Navigation or on redirect.
When postbacks are disabled, postback handlers and commit function throws an error
…tartedEventHandler from globalPostbackHandlers collections - build fix
@tomasherceg tomasherceg added this to the Version 3.0 milestone Sep 12, 2020
handled: boolean
}

type DotvvmBeforePostBackEventArgs = PostbackEventArgs & {
type DotvvmBeforePostBackEventArgs = PostbackOptions & {
cancel: boolean
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the cancel field, you can use postback handler to cancel a postback

/** Whether the new url should replace the current url in the browsing history */
readonly response?: Response
Copy link
Member

Choose a reason for hiding this comment

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

misplaced comment 🙃

}

type DotvvmStaticCommandMethodEventArgs = PostbackOptions & {
readonly command: string
Copy link
Member

Choose a reason for hiding this comment

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

I'd not include the command in the args. It is an encrypted string that may change at any moment, I doubt there is any legit use case for it.

// trigger postbackRejected event
const postbackRejectedEventArgs: DotvvmPostbackRejectedEventArgs = {
...options,
serverResponseObject,
Copy link
Member

Choose a reason for hiding this comment

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

this is redundant, when the postback is interrupted, there is no serverResponseObject and response

}
function shouldTriggerErrorEvent(err: DotvvmPostbackError) {
return !isInterruptingErrorReason(err) &&
(err.reason.type == "network" || err.reason.type == "serverError");
Copy link
Member

Choose a reason for hiding this comment

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

Simplify to err.reason.type == "network" || err.reason.type == "serverError"

serverResponseObject,
response: (err.reason as any).response,
error: err,
wasInterrupted: isInterruptingErrorReason(err),
Copy link
Member

Choose a reason for hiding this comment

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

This is always false.

serverResponseObject,
response: (err.reason as any).response,
error: err,
wasInterrupted: isInterruptingErrorReason(err),
Copy link
Member

Choose a reason for hiding this comment

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

This is always false.

type DotvvmAfterPostBackEventArgs = PostbackEventArgs & {
handled: boolean

type DotvvmAfterPostBackEventArgs = PostbackOptions & {
/** Set to true in case the postback did not finish and it was cancelled by an event or a postback handler */
readonly wasInterrupted: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

I'd chage it to wasInterrupted?: true, so we don't have to include it on all the places where it's false.

action();

const currentErrorsCount = allErrors.length;
if (originalErrorsCount === 0 && currentErrorsCount === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

We prefer == everywhere when the types match.

@tomasherceg tomasherceg merged commit f591b25 into v3-master Oct 18, 2020
@tomasherceg tomasherceg deleted the v3-postback-events-refactoring branch October 18, 2020 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants