[KZL-1098] proxify kuzzle to avoid mistyping error #395

thomasarbona · 2019-05-17T12:01:24Z

What does this PR do?

`proxify` helper

Create a helper that can throw error when get/set undeclared properties.

const obj = proxify({ foo: 42 });

console.log(obj.foo);
console.log(obj.bar); // throw

Some configuration:

proxify({ foo: 42 }, {
  name: 'object',
  seal: true,
  sealGet: false,
  deprecated: [],
  exposeApi: false,
  apiNamespace: '__proxy__',
});

name : the name of the proxified object for warnings
seal: does proxify throw error on set undeclared properties?
sealGet: does proxify throw error on get undeclared properties? this options is separated from seal because getting undefined properties can be usefull for type checking for example
deprecated: array of property names which produce a deprecate warning on get/set
warnDepreciationOnce: only warn once per deprecated property
exposeApi: expose an api in the object to manipulate properties (described below)
apiNamespace: in which namespace api is exposed

proxyfy API

const obj = proxify({ foo: 42 }, { exposeApi: true });

console.log(obj.foo); // OK
console.log(obj.bar); // throw error

obj.__proxy__.registerProp('bar');

console.log(obj.bar); // OK

const hasProp = obj.__proxy__.hasProp('baz'); // check WITHOUT warning

obj.__proxy__.unregisterProp('bar');

console.log(obj.bar); // throw error

usage on kuzzle instance

kuzzle instance use proxify like this in the constructor:

return proxify(this, {
  seal: true,
  name: 'kuzzle',
  exposeApi: true
});

How should this be manually tested?

Step 1 : instantiate Kuzzle class
Step 2 : define an undeclared property on this instance, like foo

…ascript into KZL-1098-proxify-kuzzle

scottinet

Good one.

A remark though: the documentation you put in your PR description is very nice, but it should also be in a comment of the proxify file

scottinet · 2019-05-17T13:50:58Z

src/proxify.js

+  ...options
+});
+
+const getPropertyNames = (obj) => {


(nitpicking) coding-style consistency

Suggested change

const getPropertyNames = (obj) => {

const getPropertyNames = obj => {

scottinet · 2019-05-17T14:01:53Z

src/proxify.js

+        throw new Error(`${options.name}.${name} is not defined`);
+      }
+      if (options.deprecated.includes(name)) {
+        console.warn(`Warning: ${options.name}.${name} is deprecated`);


you must add some kind of flag to trigger that warning only once per deprecated property accessed, otherwise this can clog the console up pretty quickly

scottinet · 2019-05-17T14:04:34Z

src/proxify.js

+
+  const handler = {
+    get: (target, name) => {
+      if (options.sealGet && typeof name === 'string' && !properties.includes(name)) {


you should remove the typecheck made on name:

it can only be a string or a symbol anyway, since this is a getter trap

if I pass a non-existing symbol to your handler, I won't get any error even with sealGet active, which does not seem to be the intended behavior

scottinet · 2019-05-17T14:05:25Z

src/proxify.js

+        throw new Error(`setting a not defined '${name}' properties in '${options.name}' object`)
+      }
+      if (options.deprecated.includes(name)) {
+        console.warn(`Warning: ${options.name}.${name} is deprecated`);


test/kuzzle/proxy.test.js

Co-Authored-By: Sébastien Cottinet <[email protected]>

…ascript into KZL-1098-proxify-kuzzle

src/proxify.js

Co-Authored-By: Sébastien Cottinet <[email protected]>

src/Kuzzle.js

src/proxify.js

test/kuzzle/proxy.test.js

codecov · 2019-05-17T15:26:55Z

Codecov Report

Merging #395 into 6-dev will decrease coverage by 0.02%.
The diff coverage is 95.91%.

@@            Coverage Diff             @@
##            6-dev     #395      +/-   ##
==========================================
- Coverage   96.46%   96.44%   -0.03%     
==========================================
  Files          30       31       +1     
  Lines        1442     1489      +47     
==========================================
+ Hits         1391     1436      +45     
- Misses         51       53       +2

Impacted Files	Coverage Δ
src/Kuzzle.js	`95.13% <100%> (+0.11%)`	⬆️
src/proxify.js	`95.23% <95.23%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd684cc...f6012da. Read the comment docs.

Aschen · 2019-05-18T12:40:55Z

src/proxify.js

+  const options = getOptions(opts);
+  const properties = ['inspect'];
+
+  const warnedDepreciation = new Set();


Aschen · 2019-05-18T12:43:02Z

src/proxify.js

+  }
+
+  const options = getOptions(opts);
+  const properties = ['inspect'];


Could be a Set too

src/proxify.js

Co-Authored-By: Sébastien Cottinet <[email protected]>

Release 6.1.2 Bug fixes [ #398 ] Fix bulk return (Aschen) [ #394 ] Add default values for from/size to document:search (Aschen) [ #384 ] Fix search API: "sort" and "search_after" must be in the requests body (scottinet) Enhancements [ #390 ] Add authenticated property on Kuzzle object (Aschen) [ #395 ] Proxify kuzzle to avoid mistyping error (thomasarbona) [ #389 ] Remove usage of _meta (Aschen) [ #391 ] Add isConnected (Aschen) [ #388 ] Use BaseController class for controllers (Aschen) [ #385 ] Add Security.createRestrictedUser method (Aschen) Others [ #400 ] Fix large document search using scroll (stafyniaksacha) [ #387 ] SearchResult.next returns a new instance (Aschen)

thomasarbona added 2 commits May 17, 2019 11:40

proxify kuzzle to avoid mistyping error

4866187

permit disable seal when getting properties

ccd8dc2

thomasarbona added the changelog:enhancements label May 17, 2019

thomasarbona self-assigned this May 17, 2019

thomasarbona added 3 commits May 17, 2019 14:01

Merge branch '6-dev' into KZL-1098-proxify-kuzzle

978fbf1

throw error instead of warning if get/set undeclared property

d59a309

Merge branch 'KZL-1098-proxify-kuzzle' of github.com:kuzzleio/sdk-jav…

d91e22f

…ascript into KZL-1098-proxify-kuzzle

scottinet suggested changes May 17, 2019

View reviewed changes

thomasarbona and others added 4 commits May 17, 2019 16:29

add options to warn once + some suggestions

31e7754

Update test/kuzzle/proxy.test.js

c64523f

Co-Authored-By: Sébastien Cottinet <[email protected]>

fix eslint + update proxify doc

f3d6787

Merge branch 'KZL-1098-proxify-kuzzle' of github.com:kuzzleio/sdk-jav…

3f3a171

…ascript into KZL-1098-proxify-kuzzle

thomasarbona requested a review from scottinet May 17, 2019 14:33

scottinet suggested changes May 17, 2019

View reviewed changes

thomasarbona and others added 5 commits May 17, 2019 16:51

Update src/proxify.js

c492121

Co-Authored-By: Sébastien Cottinet <[email protected]>

Update src/proxify.js

d00fa56

Co-Authored-By: Sébastien Cottinet <[email protected]>

Update src/proxify.js

2b5bc22

Co-Authored-By: Sébastien Cottinet <[email protected]>

Update src/proxify.js

d169552

Co-Authored-By: Sébastien Cottinet <[email protected]>

Update src/proxify.js

af90c3d

Co-Authored-By: Sébastien Cottinet <[email protected]>

Aschen approved these changes May 17, 2019

View reviewed changes

src/Kuzzle.js Show resolved Hide resolved

src/proxify.js Outdated Show resolved Hide resolved

src/proxify.js Outdated Show resolved Hide resolved

test/kuzzle/proxy.test.js Outdated Show resolved Hide resolved

thomasarbona added 3 commits May 17, 2019 17:05

rename warnDepreciationOnce -> warnDeprecationOnce

35efac8

replace object with Set

b0fac8c

registerProps -> registerProp / use good throwing test

b0e5ad0

thomasarbona requested a review from scottinet May 17, 2019 15:20

Aschen suggested changes May 18, 2019

View reviewed changes

src/proxify.js Outdated

}

const options = getOptions(opts);

const properties = ['inspect'];

Copy link

Contributor

Aschen May 18, 2019

Choose a reason for hiding this comment

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

Could be a Set too

scottinet suggested changes May 20, 2019

View reviewed changes

src/proxify.js Outdated Show resolved Hide resolved

thomasarbona and others added 2 commits May 20, 2019 10:52

Update src/proxify.js

500601a

Co-Authored-By: Sébastien Cottinet <[email protected]>

use Sets instead of array

5ce6157

change error message

f6012da

thomasarbona requested review from Aschen and scottinet May 20, 2019 09:33

scottinet approved these changes May 20, 2019

View reviewed changes

alexandrebouthinon approved these changes May 20, 2019

View reviewed changes

benoitvidis approved these changes May 20, 2019

View reviewed changes

Aschen approved these changes May 20, 2019

View reviewed changes

Aschen merged commit 3a77f57 into 6-dev May 20, 2019

Aschen deleted the KZL-1098-proxify-kuzzle branch May 20, 2019 15:09

Aschen mentioned this pull request Jun 14, 2019

Release 6.1.2 #402

Merged

	const getPropertyNames = (obj) => {
	const getPropertyNames = obj => {

[KZL-1098] proxify kuzzle to avoid mistyping error #395

[KZL-1098] proxify kuzzle to avoid mistyping error #395

Uh oh!

Conversation

thomasarbona commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

proxify helper

proxyfy API

usage on kuzzle instance

How should this be manually tested?

Uh oh!

scottinet left a comment

Choose a reason for hiding this comment

Uh oh!

scottinet May 17, 2019

Choose a reason for hiding this comment

Uh oh!

scottinet May 17, 2019

Choose a reason for hiding this comment

Uh oh!

scottinet May 17, 2019

Choose a reason for hiding this comment

Uh oh!

scottinet May 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Aschen May 18, 2019

Choose a reason for hiding this comment

Uh oh!

Aschen May 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thomasarbona commented May 17, 2019 •

edited

Loading

`proxify` helper

codecov bot commented May 17, 2019 •

edited

Loading