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

Massage the super method #1382

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

kriskowal
Copy link
Member

These changes fail to fix MON-234 gh-1348, but in the process add
comments, massage style, and fix theoretical internal consistency
problems introduced by thrown exceptions and nested and repeated super
calls, for which I have provided no evidence of their necessity.

These changes fail to fix MON-234 montagejsgh-1348, but in the process add
comments, massage style, and fix theoretical internal consistency
problems introduced by thrown exceptions and nested and repeated super
calls, for which I have provided no evidence of their necessity.
@@ -684,6 +764,9 @@ var getSuper = function(object, method) {
propertyName = propertyNames[i];
property = Object.getOwnPropertyDescriptor(context, propertyName);
if ((func = property.value) != null) {
// TODO These deprecatedFunction checks should be
// unnecessary if the super methods are reentrant.
// - @kriskowal
Copy link
Member Author

Choose a reason for hiding this comment

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

Feedback about this would be good if anyone has prior knowledge of the motivations here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are checking for the deprecatedFunction since the found func might be a method that is being used in "compatibility mode" i.e. a constructor method defined on the prototype.

@kriskowal
Copy link
Member Author

Notes added to assist reviewers in distinguishing style noise from substantiative changes.

@francoisfrisch
Copy link
Contributor

Will need to take the time to review the addition of the contextStack

@hthetiot hthetiot added this to the Future milestone May 1, 2017
@hthetiot hthetiot assigned hthetiot and unassigned francoisfrisch and hthetiot Jul 13, 2017
@hthetiot hthetiot requested a review from cdebost July 13, 2017 02:19
@hthetiot hthetiot modified the milestones: v17.1.x API Changes, Future Jul 13, 2017
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