-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Implement privatize, private fields on prototypes #354
base: master
Are you sure you want to change the base?
Changes from all commits
a97514a
75d860f
a924281
ddadace
07e35c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,36 @@ const mixin = (target, source) => { | |
Object.assign(target, mix); | ||
}; | ||
|
||
const ASCII_A = 65; | ||
|
||
// Convert instance with public fields to instance with private fields | ||
// instance - <Object>, source instance | ||
// Returns: <Object> - destination instance | ||
// | ||
// Example: common.privatize({ private: 5, f() { return this.private; } }); | ||
const privatize = instance => { | ||
const iface = {}; | ||
const fields = Object.keys(instance); | ||
for (const fieldName of fields) { | ||
const field = instance[fieldName]; | ||
if (typeof field === 'function') { | ||
const boundMethod = field.bind(instance); | ||
iface[fieldName] = boundMethod; | ||
} else if (fieldName === fieldName.toUpperCase()) { | ||
const first = fieldName.charCodeAt(0); | ||
if (first >= ASCII_A) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that this check is safe enough if you only wanted to limit this to uppercase Latin alphabet since the basic ASCII punctuation and symbols, such as |
||
Object.defineProperty(iface, fieldName, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Later |
||
enumerable: true, | ||
get: () => field, | ||
}); | ||
} | ||
} | ||
} | ||
return Object.freeze(iface); | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about private methods? const dest = privatize({
_privateMethod() {},
publicMethod() { this._privateMethod(); },
}); Many developers could expect that the dest contains only const ASCII_A = 'A'.charCodeAt(0);
const ASCII_Z = 'Z'.charCodeAt(0);
const isExplicitlyPrivate = name => name.charAt(0) === '_';
const isMethod = value => typeof value === 'function';
const isConstantName = name => {
if (name !== name.toUpperCase()) return false;
const firstCharCode = name.charCodeAt(0);
return firstCharCode >= ASCII_A && firstCharCode <= ASCII_Z;
};
const isPublicField = (name, value) =>
!isExplicitlyPrivate(name) &&
(isMethod(value) || isConstantName(name));
const isPrivateField = (name, value) => !isPublicField(name, value);
const privatize = (instance, isPrivate = isPrivateField) => {
const iface = {};
const fields = Object.keys(instance);
for (const fieldName of fields) {
const fieldValue = instance[fieldName];
if (isPrivate(fieldName, fieldValue)) continue; // skipping private fields
iface[fieldName] = isMethod(fieldValue)
? fieldValue.bind(instance)
: fieldValue;
}
return Object.freeze(iface);
}; |
||
|
||
module.exports = { | ||
override, | ||
mixin, | ||
privatize, | ||
}; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -62,3 +62,43 @@ metatests.test('multiple inheritance with mixin', test => { | |||||
test.strictSame(obj.property5, 'from Child.method3'); | ||||||
test.end(); | ||||||
}); | ||||||
|
||||||
metatests.test('privatize', test => { | ||||||
const source = { | ||||||
CONSTANT: 'constant value', | ||||||
field: 'field value', | ||||||
'123': 'number field value', | ||||||
counter: 0, | ||||||
method() { | ||||||
return [this.CONSTANT, this.field]; | ||||||
}, | ||||||
inc(n = 1) { | ||||||
this.counter += n; | ||||||
return this.counter; | ||||||
}, | ||||||
}; | ||||||
const destination = common.privatize(source); | ||||||
try { | ||||||
destination.CONSTANT = 'can not change freezed'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} catch (err) { | ||||||
test.strictSame(err.constructor.name, 'TypeError'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
test.strictSame(destination.CONSTANT, 'constant value'); | ||||||
} | ||||||
test.strictSame(destination.CONSTANT, 'constant value'); | ||||||
Comment on lines
+85
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of repeating the same check twice, I'd propose to catch the error and save it into a variable that can be asserted outside of the try-catch block, and then keep a check for the value of the field. let err = null;
try {
destination.CONSTANT = 'cannot change frozen';
} catch (e) {
err = e;
}
test.isError(err, new TypeError());
test.strictSame(destination.CONSTANT, 'constant value'); Though a simpler way of testing the same thing would be to call |
||||||
try { | ||||||
destination.field = 'can not change freezed'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto here, continuing on the proposal in the comment above, here we can first assert that |
||||||
} catch (err) { | ||||||
test.strictSame(err.constructor.name, 'TypeError'); | ||||||
test.strictSame(destination.field, undefined); | ||||||
} | ||||||
test.strictSame(destination['123'], undefined); | ||||||
test.strictSame(destination.method(), ['constant value', 'field value']); | ||||||
test.strictSame(destination.inc(), 1); | ||||||
try { | ||||||
destination.inc = () => 0; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||||||
} catch (err) { | ||||||
test.strictSame(err.constructor.name, 'TypeError'); | ||||||
test.strictSame(destination.inc(), 2); | ||||||
} | ||||||
test.end(); | ||||||
}); |
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.
Nested
if
doesn't cry about "let's keep constants".I guess the loop body could be refactored to: