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

introduce default type serializers #356

Closed
wants to merge 2 commits into from
Closed

introduce default type serializers #356

wants to merge 2 commits into from

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Dec 11, 2018

Fixes #264.

Maybe this is a solution?

  • The old type option has been renamed to serializer
  • type is now one of the primitive constructors (Boolean, String, Number, Object or Array) if set
  • AttributeSerializer has been renamed to ComplexAttributeSerializer
  • AttributeType has been renamed to AttributeSerializer
  • AttributeType is a type alias for the basic primitive constructors
  • Deserialization and serialization checks for a default serializer if none is specified and a type has been

This would reduce some confusion, especially for those coming from Polymer, as the type system will be almost the same (there is no support for Date). Additionally, much of the content out there about lit mistakenly uses type: Object often, so this alone tells me people are assuming that is the correct syntax already.

Examples:

@property({ type: Object })
@property({ serializer: (val) => customSerializer(val) })
@property({
  serializer: {
    toAttribute(val) { return `${val}`; },
    fromAttribute(val) { return JSON.parse(val); }
  }
});

When using something like type: Object, the following occurs:

el.myProp = { a: 5 }; // myprop='{"a":5}'
el.setAttribute('myprop', '{"a":6}'); // myProp === 6

If you have better ideas or other plans, im happy to close, just figured i'd push this since I had it.

cc @sorvell

Copy link

@admwx7 admwx7 left a comment

Choose a reason for hiding this comment

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

👍

src/lib/updating-element.ts Outdated Show resolved Hide resolved
@sorvell
Copy link
Contributor

sorvell commented Dec 14, 2018

Thanks very much for the PR, it inspired the changes in #369. There were just a few more changes desired than seemed feasible to describe in a review so we incorporated your ideas into the other PR instead.

The serializers/deserializers there are almost exactly the same. One difference is that Polymer allows Object based properties to be non-objects which seems a little loser than we want here.

Other than that, instead of serializer (I realized I suggested that), we went with converter and decided to pass type to it. This way type is really just a hint for the converter.

We welcome your feedback on #369.

@sorvell sorvell closed this Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: supported serializers out of the box
4 participants