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

add an afterRollback event #292

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

bgentry
Copy link
Contributor

@bgentry bgentry commented Mar 24, 2018

Add an afterRollback event hook

This adds a new event hook afterRollback that gets called at the end of a changeset rollback(). I wouldn't have a PR for this if I could cleanly extend the Changeset class myself, but, I can't: #178 (comment)

This is required for me to have a reasonably clean solution to offirgolan/ember-changeset-cp-validations#25 . I'll post more info there to help explain why I need this.

Cheers ✌️

@bgentry
Copy link
Contributor Author

bgentry commented May 30, 2018

Any reason not to merge this? Let me know if you want any changes.

Copy link
Collaborator

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

Just a few comments but looks like this is a good add! Would just like to hear more / documented of what the benefits to users are.

@@ -743,6 +744,20 @@ changeset.validate().then(() => {

**[⬆️ back to top](#api)**

#### `afterRollback`

This event is triggered after a rollback of the changeset.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bgentry Would you mind adding a few sentences detailing the "use case" for users? To me it isn't immediately obvious, but I trust that there is a valid use case.

addon/index.js Outdated
@@ -396,6 +397,7 @@ export function changeset(
set(this, ERRORS, {});
(this /*: ChangesetDef */)._notifyVirtualProperties(keys)

this.trigger(AFTER_ROLLBACK_EVENT);
Copy link
Collaborator

@snewcomer snewcomer Jul 12, 2018

Choose a reason for hiding this comment

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

I think to follow some other places in the code, one can do (just for reference)

let c          /*: ChangesetDef     */ = this;
c.trigger(BEFORE_VALIDATION_EVENT, key);

@bgentry
Copy link
Contributor Author

bgentry commented Jul 12, 2018

@snewcomer let me know if these changes are sufficient. The best I could do to explain it is to link to an existing post where I described why I needed this functionality.

I think it is somewhat related to the larger issue discussed in #297, which is that the changeset does not actually represent the most recently-set values for all fields (mainly those that are invalid or haven't yet completed validations).

@snewcomer
Copy link
Collaborator

@bgentry I think if you merge master in, we should get a passing test suite! The comment you added looks good enough from my end 👍

@bgentry
Copy link
Contributor Author

bgentry commented Jul 16, 2018

@snewcomer rebased as requested ✌️

@snewcomer
Copy link
Collaborator

@bgentry there might a flow error or something. Not showing the exact issue in the travis fail though

@bgentry
Copy link
Contributor Author

bgentry commented Jul 16, 2018

@snewcomer yeah, it was a flow error because the new callback does not have a 2nd string arg (i.e. the key of the attr being changed). I changed the flow signature to string | void, hopefully that's the correct choice.

@snewcomer
Copy link
Collaborator

@Dhaulagiri Mind taking a look at this PR? Increases the API surface, but does provide a valuable event for some use cases.

@snewcomer
Copy link
Collaborator

@Dhaulagiri Lmk if you have time to look at these PR's today. Going to tackle a much lgr feature after this is in ☮️

@Dhaulagiri Dhaulagiri merged commit a50fc86 into adopted-ember-addons:master Jul 24, 2018
@bgentry bgentry deleted the rollback-event branch July 24, 2018 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants