Permalink
Browse files

Refactor BaseClass to fix subsclassing and wrongly interacting proper…

…ties

Fixes framer/company#2773
  • Loading branch information...
nvh committed Jun 28, 2017
1 parent 950b059 commit 4db027525cc17a1e9bd4b41fa306fb2af654cb86
Showing with 54 additions and 37 deletions.
  1. +28 −33 framer/BaseClass.coffee
  2. +26 −4 test/tests/BaseClassTest.coffee
View
@@ -5,9 +5,13 @@ Utils = require "./Utils"
{EventEmitter} = require "./EventEmitter"
CounterKey = "_ObjectCounter"
DefinedPropertiesKey = "_DefinedPropertiesKey"
DefinedPropertiesValuesKey = "_DefinedPropertiesValuesKey"
DefinedPropertiesOrderKey = "_DefinedPropertiesOrderKey"
ObjectDescriptors = []

This comment has been minimized.

Show comment
Hide comment
@onnlucky

onnlucky Jun 28, 2017

Collaborator

How big does that ObjectDescriptor list grow? Can it not be be indexed by class? Or attached to the class?

@onnlucky

onnlucky Jun 28, 2017

Collaborator

How big does that ObjectDescriptor list grow? Can it not be be indexed by class? Or attached to the class?

This comment has been minimized.

Show comment
Hide comment
@nvh

nvh Jun 29, 2017

Collaborator

Well, every for @define there's an object descriptor, currently there are a little bit less then 400 in the Framer Library. I tried attaching it to the class, but didn't find good a solution for that in coffeescript.

@nvh

nvh Jun 29, 2017

Collaborator

Well, every for @define there's an object descriptor, currently there are a little bit less then 400 in the Framer Library. I tried attaching it to the class, but didn't find good a solution for that in coffeescript.

# Theoretically this should be an array per class, but as long as we don't do weird stuff
# like depending properties of subclasses in a different order then superclasses this will work
DefaultPropertyOrder = []
class exports.BaseClass extends EventEmitter
@@ -41,28 +45,24 @@ class exports.BaseClass extends EventEmitter
descriptor.importable ?= true
# We assume we don't import if there is no setter, because we can't
descriptor.importable = descriptor.importable and descriptor.set
descriptor.importable = descriptor.importable and descriptor.set?
# We also assume we don't export if there is no setter, because
# it is likely a calculated property, and we can't set it.
descriptor.exportable = descriptor.exportable and descriptor.set
descriptor.exportable = descriptor.exportable and descriptor.set?
# We assume that every property with an underscore is private
return if _.startsWith(propertyName, "_")
# Only retain options that are importable, exportable or both:
if descriptor.exportable or descriptor.importable
@[DefinedPropertiesKey] ?= {}
@[DefinedPropertiesKey][propertyName] = descriptor
# Set the order, insert it's dependants before, we'll check if they exist later
@[DefinedPropertiesOrderKey] ?= []
ObjectDescriptors.push([@, propertyName, descriptor])
if descriptor.depends
for depend in descriptor.depends
if depend not in @[DefinedPropertiesOrderKey]
@[DefinedPropertiesOrderKey].push(depend)
@[DefinedPropertiesOrderKey].push(propertyName)
if depend not in DefaultPropertyOrder
DefaultPropertyOrder.push(depend)
if propertyName not in DefaultPropertyOrder
DefaultPropertyOrder.push(propertyName)
@simpleProperty = (name, fallback, options={}) ->
return _.extend options,
@@ -97,7 +97,11 @@ class exports.BaseClass extends EventEmitter
@_propertyList()[k]["default"]
_propertyList: ->
@constructor[DefinedPropertiesKey]
result = {}
for k in ObjectDescriptors
if @ instanceof k[0]
result[k[1]] = k[2]
return result
keys: -> _.keys(@props)
@@ -151,36 +155,27 @@ class exports.BaseClass extends EventEmitter
# we don't get confused because the id changes from global to context
@_id = @constructor[CounterKey]
_applyDefaults: (options) ->
_applyDefaults: (options, proxy = false) ->
return unless @constructor[DefinedPropertiesOrderKey]
return unless options
for k in @constructor[DefinedPropertiesOrderKey]
@_applyDefault(k, options[k])
propertyList = @_propertyList()
for k in DefaultPropertyOrder
descriptor = propertyList[k]
if descriptor?
continue if proxy and not (descriptor.proxy is true)
@_applyDefault(descriptor, k, options[k])
_applyProxyDefaults: (options) ->
@_applyDefaults(options, true)
return unless @constructor[DefinedPropertiesOrderKey]
return unless options
for k in @constructor[DefinedPropertiesOrderKey]
descriptor = @constructor[DefinedPropertiesKey][k]
continue unless descriptor?.proxy? is true
@_applyDefault(k, options[k])
_applyDefault: (key, optionValue) ->
descriptor = @constructor[DefinedPropertiesKey][key]
# If this was listed as a dependent property, but it did not get defined, we err.
throw Error("Missing dependant descriptor: #{key}") unless descriptor
_applyDefault: (descriptor, key, optionValue) ->
# For each known property (registered with @define) that has a setter, fetch
# the value from the options object, unless the prop is not importable.
# When there's no user value, apply the default value:
return unless descriptor.set
return unless descriptor.set?
value = optionValue if descriptor.importable
value = Utils.valueOrDefault(optionValue, @_getPropertyDefaultValue(key))
@@ -207,7 +207,7 @@ describe "BaseClass", ->
instance.testPropA.should.equal "a"
instance.testPropB.should.equal "value"
it.skip "should have defined properties set in sibling subclasses", ->
it "should have defined properties set in sibling subclasses", ->
class LalaLayer extends Framer.BaseClass
@define "blabla",
@@ -225,11 +225,13 @@ describe "BaseClass", ->
get: -> "getClassC"
# set: -> "setClassC"
expect(TestClassD["_DefinedPropertiesKey"]?.a?.set).to.be.ok
expect(TestClassC["_DefinedPropertiesKey"]?.a?.set).to.not.be.ok
d = new TestClassD
c = new TestClassC
expect(d._propertyList()?.a?.set).to.be.ok
expect(c._propertyList()?.a?.set).to.not.be.ok
it.skip "should not export a shared property name in props of in sibling subclasses", ->
it "should not export a shared property name in props of in sibling subclasses", ->
class BaseSubClass extends Framer.BaseClass
@define "blabla",
@@ -255,3 +257,23 @@ describe "BaseClass", ->
expect(b.blabla).to.be.ok
expect(a.props.blabla).to.be.ok
expect(b.props.blabla).to.be.ok
it "should allow overrides of properties", ->
class TestA extends Framer.BaseClass
@define "test", @simpleProperty("test", "a")
class TestB extends TestA
@define "test", @simpleProperty("test", "b")
a = new TestA
b = new TestB
a.test.should.equal "a"
b.test.should.equal "b"
it "should inherit properties", ->
class TestInherit extends Framer.BaseClass
@define "test", @simpleProperty("test", "a")
class TestInheritB extends TestInherit
a = new TestInherit
b = new TestInheritB
test: null
a.test.should.equal "a"
b.test.should.equal "a"

0 comments on commit 4db0275

Please sign in to comment.