Skip to content

Basic tests for private methods#1963

Merged
rwaldron merged 5 commits into
tc39:masterfrom
leobalter:private-methods-continue
Nov 26, 2018
Merged

Basic tests for private methods#1963
rwaldron merged 5 commits into
tc39:masterfrom
leobalter:private-methods-continue

Conversation

@leobalter

Copy link
Copy Markdown
Member

No description provided.

@leobalter leobalter requested a review from rwaldron November 16, 2018 19:06

@rwaldron rwaldron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See inline comments

* the template provides c.ref() for external reference
*/

function hasOwnProperty(obj, name) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you just use Reflect.has(obj, name) instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Interesting... using Reflect.has I can also confirm the private method is not in the prototype chain. I wonder if I should use both.

---*/

/***
* template notes:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This new practice is excellent—thanks!

/*---
description: Private Async Generator (private method definitions in a class expression)
esid: prod-MethodDefinition
features: [async-iteration]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing class-methods-private

* 1. Let methodDef be DefineMethod of MethodDefinition with argument homeObject.
* ...
*/
assert.sameValue(c.ref, other.ref, 'The method is defined once, and reused on every new instance');

@rwaldron rwaldron Nov 19, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1. ref isn't a method
2. I'm certain these semantics are already well tested

'private methods are defined in an special internal slot and cannot be found as own properties'
);
assert.sameValue(typeof this.#m, 'function');
assert.sameValue(this.ref(), this.#m, 'returns the same value');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ref is an accessor, so this should be assert.sameValue(this.ref, this.#m, 'returns the same value');.


assert.sameValue(c.ref.name, '#m', 'function name is preserved external reference');
ctorPromise.then(() => {
var iter = c.ref();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be helpful if there was a comment that accompanied this line, which noted that this is actually calling this.#m

'private methods are defined in an special internal slot and cannot be found as own properties'
);
assert.sameValue(typeof this.#m, 'function');
assert.sameValue(this.ref(), this.#m, 'returns the same value');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@leobalter

Copy link
Copy Markdown
Member Author

I pushed the changes, please review again, @rwaldron

@rwaldron rwaldron merged commit 9d5aa7d into tc39:master Nov 26, 2018
@leobalter leobalter deleted the private-methods-continue branch November 26, 2018 21:40
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.

2 participants