Skip to content

Conversation

@brianpeiris
Copy link
Contributor

@brianpeiris brianpeiris commented Dec 19, 2017

Description:
Currently most functions in an A-Frame call stack are named "value" due to the way elements are defined and registered.
This change makes use of the displayName property on functions to improve call stack readability.

Before:
Debugger call stack with multiple functions named "value"

After:
Debugger call stack with properly named functions

Changes proposed:

  • Name functions on registered elements using the displayName property.

}

// Give all functions their proper name.
for (const propName of Object.getOwnPropertyNames(newObj.prototype)) {
Copy link
Member

Choose a reason for hiding this comment

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

not an es6 codebase

document.body.appendChild(document.createElement('a-test-entity-2'));
});

test.only('Names functions correctly', function (done) {
Copy link
Member

Choose a reason for hiding this comment

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

remove .only, uncapitalize test name

Copy link
Member

@ngokevin ngokevin left a comment

Choose a reason for hiding this comment

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

thanks!

// Give all functions their proper name.
for (var propName of Object.getOwnPropertyNames(newObj.prototype)) {
var propVal = newObj.prototype[propName];
if (propVal && typeof propVal === 'function') {
Copy link
Member

Choose a reason for hiding this comment

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

Is checking for if ( typeof propVal === 'function') sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. Will fix.

@dmarcos
Copy link
Member

dmarcos commented Dec 19, 2017

There's a test.only left to remove and this is mergeable:

#3310 (diff)

@brianpeiris
Copy link
Contributor Author

The test.only was removed in 829dcc0.

@dmarcos
Copy link
Member

dmarcos commented Dec 19, 2017

You're right! The Github PR conversation is not the best to keep track of updates.

@dmarcos dmarcos merged commit f5ef24b into aframevr:master Dec 19, 2017
@dmarcos
Copy link
Member

dmarcos commented Dec 19, 2017

Thanks! 👍

@brianpeiris
Copy link
Contributor Author

Sweet, thanks.

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.

3 participants