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 support for types #26

Merged
merged 4 commits into from
May 15, 2019
Merged

Add support for types #26

merged 4 commits into from
May 15, 2019

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented May 14, 2019

This adds support for types as defined by can-data-types. This is
dependant on a new minor version of can-data-types with these methods
added.

Strict types

This is provided by type: types.check(Type) or by type: Type for the case of native types like Date, String, Number.

Here's an example usage:

class Person extends DefineClass {
  static define = {
    age: {
      type: Number
    },
    birthday: {
      type: types.check(Date)
    }
  }
}

In the above, it will throw if a non-number age is provided or a
non-Date birthday.

Maybe types

This is provided by type: types.maybe(Type). This is a strict type,
but null and undefined` are also allowed.

class Person extends DefineClass {
  static define = {
    age: {
      type: Number
    },
    birthday: {
      type: types.maybe(Date)
    }
  }
}

Convert / coerce

This is provided by type: types.convert(Type) and accepts any type,
which is attempted to be coerced to Type.

class Person extends DefineClass {
  static define = {
    age: {
      type: types.convert(Number)
    }
  }
}

let person = new Person({ age: "23" });
// person.age -> 23

Maybe convert

Like types.maybe() but attempts to convert. Values of null and
undefined will be left alone.

Required properties

This pull request also adds a required property. When set to true a
value must be provided upon instantiation.

class Person extends DefineClass {
  static define = {
    age: {
      type: Number,
      required: true
    }
  }
}

let person = new Person();
// Throws with a message about age being required.

Weird things

  • This is built on top of canReflect.convert(). It's a little weird that strict types are implemented throw a convert() function. It's infrastructure so I tend to think this doesn't matter as much.

Pending

  • Should convert be coerce and maybeConvert be maybeCoerce?
  • Currently instantiation happens upon construction. For custom elements
    this needs to be delayed until connectedCallback. This will be added
    in a followup issue.
  • type: Primitive is not yet implemented.
  • prop: Type is not yet implemented. Should it be?

Closes #3

This adds support for types as defined by can-data-types. This is
dependant on a new minor version of can-data-types with these methods
added.

# Strict types

This is provided by `type: types.check(Type)` or by `type: Type` for the case of native types like Date, String, Number.

Here's an example usage:

```js
class Person extends DefineClass {
  static define = {
    age: {
      type: Number
    },
    birthday: {
      type: types.check(Date)
    }
  }
}
```

In the above, it will throw if a non-number `age` is provided or a
non-Date `birthday`.

# Maybe types

This is provided by `type: types.maybe(Type)`. This is a strict type,
	 but `null and `undefined` are also allowed.

```js
class Person extends DefineClass {
  static define = {
    age: {
      type: Number
    },
    birthday: {
      type: types.maybe(Date)
    }
  }
}
```

# Convert / coerce

This is provided by `type: types.convert(Type)` and accepts any type,
	 which is attempted to be coerced to Type.

```js
class Person extends DefineClass {
  static define = {
    age: {
      type: types.convert(Number)
    }
  }
}

let person = new Person({ age: "23" });
// person.age -> 23
```

# Maybe convert

Like `types.maybe()` but attempts to convert. Values of `null` and
`undefined` will be left alone.

# Required properties

This pull request also adds a `required` property. When set to true a
value __must__ be provided upon instantiation.

```js
class Person extends DefineClass {
  static define = {
    age: {
      type: Number,
      required: true
	}
  }
}

let person = new Person();
// Throws with a message about age being required.
```

# Pending

* Should __convert__ be __coerce__ and __maybeConvert__ be __maybeCoerce__?
* Currently instantiation happens upon construction. For custom elements
this needs to be delayed until `connectedCallback`. This will be added
in a followup issue.
* `type: Primitive` is not yet implemented.
* `prop: Type` is not yet implemented. Should it be?
Copy link
Contributor

@phillipskevin phillipskevin left a comment

Choose a reason for hiding this comment

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

This is built on top of canReflect.convert().

Where does this happen? Or is this not in the code yet?

Should convert be coerce and maybeConvert be maybeCoerce?

I prefer convert due to how some people have trouble spelling coerce.

Currently instantiation happens upon construction. For custom elements
this needs to be delayed until connectedCallback. This will be added
in a followup issue.

Is this something that can-define-class should be responsible for? can-stache-define-element will use the define function directly to do the instantiation in the connectedCallback.

if(requiredButNotProvided.size) {
var msg, missingProps = Array.from(requiredButNotProvided);
if(requiredButNotProvided.size === 1) {
msg = `Missing required property [${missingProps[0]}].`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the name of the type to this error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that we should, the required property is unrelated to the type. A property can be required without having an explicit type. I think adding the type might make it confusing, but I also see how it might be useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant the class, like canReflect.getName(this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, so if you are rendering a template with many components you can figure out which one it throwing the error? Makes sense. Probably should check for localName possibly even.

@matthewp
Copy link
Contributor Author

Where does this happen? Or is this not in the code yet?

Already in the code ported from can-define. https://github.com/canjs/can-define-class/blob/f9ba6177fe849d135979b2e0f479e40b6f4109aa/define.js#L623

Is this something that can-define-class should be responsible for? can-stache-define-element will use the define function directly to do the instantiation in the connectedCallback.

Not sure how that's possible... the symbols and everything else on the prototype needs to be there. So it needs to be used as a class mixin. Plus I definitely think can-define-class should be useable directly on an HTMLElement.

@matthewp
Copy link
Contributor Author

I'm going to do a prerelease of can-data-types and then update this.

@matthewp matthewp marked this pull request as ready for review May 15, 2019 18:19
@matthewp matthewp merged commit 4d223ab into master May 15, 2019
@matthewp matthewp deleted the types branch May 15, 2019 19:50
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.

Make sure it works with can-data-types
2 participants