shape.loose does not work

[EyMaddis]

Hi again,
I noticed that shape({}).loose does not work. I did some digging and noticed that the unit test is actually incorrect.

I adapted the test to be breaking:

   it('should validate an object with keys NOT present in the shape on `loose` mode', () => {

      const customType = VueTypes.shape(shape).loose
      expect(customType.validator({
        id: 10,
        name: 'John',
        newProperty: true
      })).toBe(true)
    })

Test result:

WARN: '[VueTypes warn]: object is missing "newProperty" property'
[...]
SUMMARY:
✔ 76 tests completed
✖ 1 test failed

FAILED TESTS:
  VuePropTypes
    `.shape`
      ✖ should validate an object with keys NOT present in the shape on `loose` mode
        PhantomJS 2.1.1 (Mac OS X 0.0.0)
      Expected false to be true
      assert@test/index.spec.js:734:27
      toBe@test/index.spec.js:2615:28
      test/index.spec.js:5672:15

The problem is that you use objectAssign({ isLoose: true }, this) (see here).
You create a new object, instead of appending data to this, you create a new object. This object is however not the one which is bound to the .shape() function.
If loose would set this.isLoose=true, it is fixed, but I think the new object is actually intended behavior (that's why this is not a PR).

Actually the fix from #5 is the problem, because this binds the context of the validator function.

[dwightjack]

Hi,

you're right. Looks like I messed it up.

In the end the only solution seems to set isLoose without creating a new object.
Since that validator function it's the only place where I refer to this, I've reverted the previous change by binding the context just on the shape().loose modifier.

About creating a new object: I wanted to be as spec compliant as possible to the Vue types definition, without introducing new properties, removing any def, isRequired etc getter.
Maybe that's too much thinking since the are not enumerable and won't harm...

[dwightjack]

By the way. I missed the important part: pushed a new version, let me know if it fixes the problem... sorry for the mess!

[EyMaddis]

@dwightjack It of course makes .loose work, but reopens #5 when not using .loose. Your tests are actually not correct on a bigger scale.
I opened PR #7 to fix this once and for all :)

[dwightjack]

Merged.

Thanks for contributing. :-)

[EyMaddis]

closed through #7

[EyMaddis]

@dwightjack don't forget https://github.com/dwightjack/vue-types/releases ;)

[dwightjack]

Yep sure.

I was thinking about a edge case. If i call something like that:

shape({ ... }).loose.isRequired

The isRequired flag will remove isLoose flags (since they are not enumerable). The solution could be here to ovewrite the validator function in the .loose flag. This way we'll get rid of the this issue. See https://github.com/dwightjack/vue-types/blob/feature/loose/src/index.js#L223

On the other hand I'm wondering if getting rid of the objectAssign thing will be a better option for cleaner and more predictable code...

[EyMaddis]

The isRequired flag will remove isLoose flags (since they are not enumerable).

So you mean if there would be multiple flags, like .loose.isRequired.somethingElse?

By the way, another issue is that the order is important.
I will add a breaking test as a PR.

[dwightjack]

That's the main issue. On .isRequired and .def I'm duplicating the object with just the enumerable properties. Setting _VueTypes_isLoose as enumerable should fix it.

Overwriting the validator function will keep the object clean but the code itself smells...

[EyMaddis]

@dwightjack Are you sure that this is an issue?
It should be bound to the same this context. So the isLoose is not "visible" in the object that gets returned from isRequired, but it should still work. The first test in the PR #8 is actually proofing that.

Edit: you were faster than this second response, so this relates to my previous comment

[EyMaddis]

Why do you duplicate it at all?
I added the enumerable: false, just so that Vue cannot see it.

By the way, you maybe could avoid the copying altogether if you prototype-link all the flags. I will write this down in more detail later, as I have to go now.