Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "function" type to can-define #392

Open
justinbmeyer opened this issue Sep 19, 2018 · 5 comments
Open

Add "function" type to can-define #392

justinbmeyer opened this issue Sep 19, 2018 · 5 comments

Comments

@justinbmeyer
Copy link
Contributor

justinbmeyer commented Sep 19, 2018

Passing functions is much more common with CanJS. I'd like to add a "function" type so these properties can be clearly identified.

Instead of:

{
  update: "any"
}

We'd be able to write:

{
  update: "function"
}

Or:

import {MaybeFunction} from "can";

{
  update: MaybeFunction
}

Questions

What should happen if people pass something other than null, undefined, or a function?

  • Warn and convert to null?
  • Throw?
  • Warn and convert to a no-op?
  • Convert to a no-op that warns?
    Type = DefineMap.extend({func: "function"});
    var instance = new Type({func: "NOT A FUNCTION"});
    instance.func() //-> console.warns( "func was set to 'NOT A FUNCTION'.");

How should we handle default values?

Should a default look like this to be consistent?

DefineMap.extend({
  update: {
    type: "function",
    default() {
      return function() { ... }
    }
  }
});

What should we do with methods that are set?

var Type = DefineMap.extend({
  update(){}
});

var instance = new Type();

instance.update = function(){} // throws "Cannot add property update, object is not extensible"

We probably should not make update() a settable property because that would mean we have to make .update() always go through a getter and call Observation.add(). It's probably not worth the performance cost.

@thomaswilburn
Copy link
Contributor

I don't know what the existing behavior for a type mismatch is, but I would expect it to throw.

@justinbmeyer
Copy link
Contributor Author

@thomaswilburn the existing behavior is coercion. For example,

var MyMap = DefineMap.extend({age: "number"});

new MyMap({age: "21"}).age //-> 21;
new MyMap({age: "foo"}).age //-> NaN

We don't throw. For that, there is this proposal: #334

@matthewp
Copy link
Contributor

How would you define the default value for this?

DefineMap.extend({
  update: {
    type: "function",
    default() {
      return function() { ... }
    }
  }
});

vs. how you would do it now:

DefineMap.extend({
  update() {
    // ...
  }
});

@justinbmeyer
Copy link
Contributor Author

@matthewp we could special case a function type's default. But that feels weird.

@mjstahl
Copy link
Contributor

mjstahl commented Oct 11, 2018

What should happen if people pass something other than null, undefined, or a function?

I think the value should still be coerced (I assume something like new Function(4), assume 4 was the value passed), and we should warn the User. If we can tell them the the component, property name, and incorrect value passed that would be awesome.

How should we handle default values?

I think a special syntax (update() {}) would be okay but is not necessary at this stage (in general I find the less special cases the better -- less cognitive overhead, less surprises, less required documentation). default for this type should perform as it does for all the other types.

DefineMap.extend({
  update: {
    type: "function",
    default() {
      return function() {};
    }
  }
});

What should we do with methods that are set?

I /do not/ think methods should be able to be redefined. If you want to change out functions on a DefineMap, make it a property and not a method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants