flow-runtime icon indicating copy to clipboard operation
flow-runtime copied to clipboard

generated getters/setters for class property assertions break property ownership

Open RuBAN-GT opened this issue 7 years ago • 7 comments

This is a:

  • [x] Bug Report
  • [ ] Feature Request
  • [ ] Question
  • [ ] Other

Which concerns:

  • [x] flow-runtime
  • [x] babel-plugin-flow-runtime
  • [ ] flow-runtime-validators
  • [ ] flow-runtime-mobx
  • [ ] flow-config-parser
  • [ ] The documentation website

When I trying to clone my object with lodash clone method, it loses values of object properties:

// @flow

import { clone } from 'lodash'

class A {
  prop: number

  constructor(prop: number) {
    this.prop = prop
  }
}

test('cloning of typed objects', () => {
  const a = new A(42)

  const copy = clone(a)

  expect(a).not.toBe(copy)
  expect(copy.prop).toBe(42) 
})

With example above I got a error from jest:

 FAIL  a.spec.js
  ✕ cloning of typed objects (6ms)

  ● cloning of typed objects

    expect(received).toBe(expected)
    
    Expected value to be (using ===):
      42
    Received:
      undefined

What is the expected behaviour?

It should returns cloned object with setted properties and works correctly with lodash, because it's popular package from npm.


Which package versions are you using?

  • flow-runtime - 0.16
  • babel-plugin-flow-runtime - 0.15
  • lodash - 4

RuBAN-GT avatar Jan 17 '18 10:01 RuBAN-GT

I was unable to reproduce this. Would you mind putting your package.json, yarn.lock or package-lock.json if any, .babelrc, and .flowconfig into a gist?

jedwards1211 avatar Jan 22 '18 15:01 jedwards1211

@jedwards1211, yes, I move part of my project to repository https://github.com/RuBAN-GT/flow-sample.

To see this error you should run the command:

yarn run test

Thank you for your help!

RuBAN-GT avatar Jan 23 '18 13:01 RuBAN-GT

Thanks for the code @RuBAN-GT, I was able to reproduce it!

The reason this is happening is that babel-plugin-flow-runtime essentially calls Object.defineProperty(A.prototype, 'prop', {...}) to establish a getter and setter that perform runtime type checks.

But lodash.cloneDeep only Because the property is defined on A.prototype rather than an instance of A, it doesn't show up as an own property of the instance of A when lodash is cloning it.

Until we fix this you could work around this by using cloneDeepWith on any object tree that contains instances of A, like so:

function customizer(value) {
  if (value instanceof A) {
    return new A(value.prop)
  }
}
 

var clonedStuff = cloneDeepWith(stuff, customizer);

So @phpnode I think babel-plugin-flow-runtime should be calling Object.defineProperty(this, 'prop', {...}) in A's constructor rather than on A.prototype. Would you agree?

jedwards1211 avatar Jan 23 '18 16:01 jedwards1211

@phpnode because this bug causes the prototype to own class properties instead of the class itself it could cause quite a few other issues as well.

The simplest way to demonstrate this is that Object.keys(new A()) outputs an empty array, rather than ['prop'] as it should.

jedwards1211 avatar Jan 23 '18 16:01 jedwards1211

Yes I think so, this seems to be a consequence of using decorators for this. I think at this point we should switch to doing this work in the constructor and drop the requirement for decorators entirely (we can still support their use, but we don't need em). I also noticed that we don't even do property validation when a class property doesn't have an initial value, which is a slight oversight!

phpnode avatar Feb 07 '18 23:02 phpnode

Yeah that's true, I can't think of a way to do it with property decorators either. I had a bit of trouble piecing together how all the resulting code gets generated. Any tips on where I should look?

jedwards1211 avatar Feb 08 '18 05:02 jedwards1211

@jedwards1211 yeah it's a little bit tricky / messy, this particular transform happens in https://github.com/codemix/flow-runtime/blob/master/packages/babel-plugin-flow-runtime/src/transformVisitors.js#L789

phpnode avatar Feb 08 '18 10:02 phpnode