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

Implement privatize, private fields on prototypes #354

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ and this project adheres to

## [Unreleased][unreleased]

- Implement privatize, private fields on prototypes: `privatize(instance)`

## [2.2.0][] - 2020-07-10

### Added
Expand Down
15 changes: 15 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ $ npm install @metarhia/common
- [parseHost](#parsehosthost)
- [override](#overrideobj-fn)
- [mixin](#mixintarget-source)
- [privatize](#privatizeinstance)
- [Pool](#class-pool)
- [Pool.prototype.constructor](#poolprototypeconstructorfactory--null)
- [Pool.prototype.get](#poolprototypeget)
Expand Down Expand Up @@ -1334,6 +1335,20 @@ Previous function will be accessible by obj.fnName.inherited

Mixin for ES6 classes without overriding existing methods

### privatize(instance)

- `instance`: [`<Object>`][object] source instance

_Returns:_ [`<Object>`][object] - destination instance

Convert instance with public fields to instance with private fields

_Example:_

```js
common.privatize({ private: 5, f() { return this.private; } });
```

### class Pool

#### Pool.prototype.constructor(factory = null)
Expand Down
1 change: 1 addition & 0 deletions common.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ export const localIPs = common.localIPs;
export const parseHost = common.parseHost;
export const override = common.override;
export const mixin = common.mixin;
export const privatize = common.privatize;
export const Pool = common.Pool;
export const sortComparePriority = common.sortComparePriority;
export const sortCompareDirectories = common.sortCompareDirectories;
Expand Down
29 changes: 29 additions & 0 deletions lib/oop.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link

@DScheglov DScheglov Nov 11, 2020

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:

const field = instance[fieldName];
const isMethod = typeof field === 'function';
if (!isMethod && !isConstantName(fieldName)) continue; // skipping private fields
iface[fieldName] = isMethod ? field.bind(instance) : field;

const first = fieldName.charCodeAt(0);
if (first >= ASCII_A) {
Copy link
Member

Choose a reason for hiding this comment

The 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 '[', '\', ']', '^', '_', '`' will not change when put into uppercase and have their code value higher than the code value of 'Z', thus keys containing one of those characters in the starting position might be incorrectly mapped. I would propose to clearly specify the ends of the range of the characters that such keys are supposed to start with.

Object.defineProperty(iface, fieldName, {

Choose a reason for hiding this comment

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

Later Object.freeze(iface); covers this property definition.

enumerable: true,
get: () => field,
});
}
}
}
return Object.freeze(iface);
};
Copy link

@DScheglov DScheglov Nov 11, 2020

Choose a reason for hiding this comment

The 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 publicMethod.
So, I suggest to extract logic of private fields recogintion to the dedicated function and pass it to the privatize as a parameter.

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,
};
40 changes: 40 additions & 0 deletions test/oop.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
destination.CONSTANT = 'can not change freezed';
destination.CONSTANT = 'cannot change frozen';

} catch (err) {
test.strictSame(err.constructor.name, 'TypeError');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test.strictSame(err.constructor.name, 'TypeError');
test.isError(err, new TypeError());

test.strictSame(destination.CONSTANT, 'constant value');
}
test.strictSame(destination.CONSTANT, 'constant value');
Comment on lines +85 to +87
Copy link
Member

Choose a reason for hiding this comment

The 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 Object.getOwnPropertyDescriptor() for that property, and test that descriptor object doesn't have writable or configurable set to true and has set set to undefined.

try {
destination.field = 'can not change freezed';
Copy link
Member

Choose a reason for hiding this comment

The 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 Object.getOwnPropertyDescriptor() returns undefined and then check the result of calling Object.isExtensible() which is expected to be false, meaning that this field does not exist and it cannot be added.

} 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;
Copy link
Member

Choose a reason for hiding this comment

The 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();
});