-
-
Notifications
You must be signed in to change notification settings - Fork 770
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
Set wrappedMethod on getters/setters #2251
Conversation
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Once this is merged, we should reverse the #2270 changes! |
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.
Thank you for diligently fixing this issue and for your patience with waiting for a review 🏆
I've added a few suggestions around using forEach
instead of for
loops.
I'll be taking this across the finish line this week (not so heroic as it seems: basically just implementing the feedback). |
Running the tests in browsers, @hexeberlin and I discovered that this PR fails in IE11 and Safari 9
Since we will drop IE11 with #2324 and subsequently allow for more modern syntax and probably require ES5 support as minimum, I propose that we delay fixing #2198 until we've had a chance to refactor |
Not totally sure what should happen, but we have dropped support for IE11 and Safari 9 with Sinon 10, so ... how do we progress from here? I not totally sure what is supposed to happen first. |
I think we should try rebasing it onto the primary branch, and see if that doesn't fix it. I don't currently have the bandwidth to do that, but might be able to look into it on the weekend. |
Rebased: https://github.com/sinonjs/sinon/compare/master...fatso83:SINON-2198?expand=1 Some conflicts, but I think it's ok. All tests pass, at least. |
Resubmitted as #2378 to get it in. |
Purpose
This PR fixes #2198 by wrapping accessors if they are present, rather than the property they belong to.
Background
Because the current wrapMethod implementation only assumes the presence of a single method on a property, trying to set both a getter and a setter will cause the former to be overwritten by the latter, and not added as separate
get
andset
methods. Furthermore,restore()
is implemented on the property itself, which does not allow thespy.get.restore()
andspy.set.restore()
outlined in the documentation.Solution
This problem was addressed by changing wrapMethod to handle multiple methods per property. This includes a check whether any particular method is an accessor or not, and a method-wrapping implementation is chosen based on this. If the method in question is not a "get" or "set" method, the existing implementation will be used.
In order to satisfy ESLint code quality requirements, some restructuring of wrapMethod has been performed as well. To the extent possible, however, the existing structure has been left alone.
How to verify
npm install
npm test
(See included test for details)