Skip to content

Conversation

GHEMID-Mohamed
Copy link
Collaborator

No description provided.

@GHEMID-Mohamed GHEMID-Mohamed requested a review from julien-f March 12, 2019 13:23
src/new.spec.js Outdated

expect(getRenderArgs()[0]).toHaveProperty("effects");
expect(getRenderArgs()[0]).toHaveProperty("resetState");
expect(getRenderArgs()[0]).toHaveProperty("state");
Copy link
Collaborator

@julien-f julien-f Mar 12, 2019

Choose a reason for hiding this comment

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

What do you think about this?

const isReadOnly = object =>
  !Object.isExtensible(object) &&
  Object.getOwnPropertyNames(object).every(name => {
    const descriptor = Object.getOwnPropertyDescriptor(object, name)
    return (
      !descriptor.configurable &&
      (descriptor.set === undefined || !descriptor.writable)
    )
  })
const store = getRenderArgs()[0]
assert(isReadOnly(store))

const { effects, resetState, state } = store

assert(isReadOnly(effects))
expect(Object.getOwnPropertyNames(effects)).toEqual(['_setState', 'myEffect'])

expect(typeof resetState).toBe('function')

assert(isReadOnly(state))
expect(Object.getOwnPropertyNames(state)).toEqual(['myEntry'])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see that this is very precise. but it's not related with what was the described in the test: receives the store as first param with effects, state and resetState
I will add what you wrote in a new test

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I described was just a list of ideas of what to test.

Feel free to add, merge and split as necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

const isReadOnly = object =>
  !Object.isExtensible(object) &&
  Object.getOwnPropertyDescriptors(object).every(
    _ => _.set === undefined || _.writable === false
  )

Every works only with arrays and not with objects

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, you should use Object.keys() with Object.getOwnPropertyDescriptor() then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isReadOnly should be like this:

const isReadOnly = object =>
  !Object.isExtensible(object) &&
  Object.values(Object.getOwnPropertyDescriptors(object)).every(
    _ => _.set === undefined || _.writable === false
  );

and Object.keys(effects) will always be [] (empty) because nothing is enumerable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

46:3   error  Object.values() is not supported in IE 11                                                                           compat/compat
46:3   error  The 'Object.values' is not supported until Node.js 7.0.0. The configured version range is '>=6'                     node/no-unsupported-features/es-builtins
46:17  error  The 'Object.getOwnPropertyDescriptors' is not supported until Node.js 7.0.0. The configured version range is '>=6'  node/no-unsupported-features/es-builtins

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right for Object.keys, I should have said Object.getOwnPropertyNames.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Easier solution is to use Object.getOwnPropertyNames and Object.getOwnPropertyDescriptor.

@JsCommunity JsCommunity deleted a comment from GHEMID-Mohamed Mar 12, 2019
@JsCommunity JsCommunity deleted a comment from GHEMID-Mohamed Mar 12, 2019
@GHEMID-Mohamed GHEMID-Mohamed requested a review from julien-f March 12, 2019 16:05
src/new.spec.js Outdated

describe("withStore", () => {
describe("render function", () => {
it("returns readOnly state, effects, props and resetState func", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

!

@julien-f julien-f merged commit 85ea3ad into JsCommunity:next Mar 12, 2019
julien-f pushed a commit that referenced this pull request Mar 21, 2019
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