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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cherry-picked fix for attributes syncronization issue onto 0.15 branch #2063

Merged

Conversation

naz
Copy link
Contributor

@naz naz commented Mar 24, 2020

This PR introduced changes that came with #2062 into 0.15 branch as discussed here.

@ricardograca let me know if there's anything else is needed here 馃槈

naz added 2 commits Mar 24, 2020
closes bookshelf#2007

- The attributes object can be modified within `triggerThen` through hooks. Sample scenario described here bookshelf#2007 (comment).
- Recalculating 'attributesToSave' object in after-triggerThen step solves the syncronization issue because it is still based on model's `attributes` property (the reference is not lost)
@naz
Copy link
Contributor Author

naz commented Mar 24, 2020

The linting error seems rather strange. It fails on a file that hasn't ever been touched 馃

@ricardograca
Copy link
Member

ricardograca commented Mar 24, 2020

The linting error seems rather strange. It fails on a file that hasn't ever been touched thinking

Could be due to a newer version of the linter or its rules. Do you mind applying the proposed changes anyway?

@ricardograca ricardograca linked an issue Mar 24, 2020 that may be closed by this pull request
@naz
Copy link
Contributor Author

naz commented Mar 24, 2020

Sure, don't mind putting them in. Will update soon

@naz
Copy link
Contributor Author

naz commented Mar 25, 2020

Think everything is ready for this one to go into 0.15 branch @ricardograca :)

Copy link
Member

@ricardograca ricardograca left a comment

Thanks a lot. I'll prepare both releases later today.

@ricardograca ricardograca merged commit 1079783 into bookshelf:0.15 Mar 25, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants