Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants