Skip to content

fixed decorators to return a property descriptor to work with babel d… #206

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

Closed
wants to merge 2 commits into from

Conversation

mzeiher
Copy link

@mzeiher mzeiher commented Sep 18, 2018

…ecorators

Reference Issue

Fixes #205

@mzeiher mzeiher requested a review from sorvell as a code owner September 18, 2018 15:33
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@mzeiher
Copy link
Author

mzeiher commented Sep 18, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@aadamsx
Copy link

aadamsx commented Sep 30, 2018

Can't add decorators to our classes right now using Bable7. Does this fix that issue?

export const property = (options?: PropertyDeclaration) => (proto: Object,
name: string) => {
(proto.constructor as typeof UpdatingElement).createProperty(name, options);
export const property = (options?: PropertyDeclaration): FixedPropertyDecorator => (proto: Object,
Copy link
Contributor

Choose a reason for hiding this comment

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

to satisfy typescript (temporarily) and be consistent with how other decorators are written, this should probably return any.

get() { return this[key]; },
set(value) {

let defaultInitializer: null | (() => any) = descriptor ? descriptor!.initializer || null : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole handling of the default value seems just a little messy but i can see why...

The default value really needs to be in the constructor but babel annoyingly doesn't put it there...

@mzeiher
Copy link
Author

mzeiher commented Oct 3, 2018

@43081j I found away to support all, stage-0 babel, stage-0 typescript and stage-2 babel decorators while experimenting in my own decorator project: https://github.com/mzeiher/ce-decorators/blob/master/src/stage2decorators.ts, https://github.com/mzeiher/ce-decorators/blob/master/src/prop.ts, should in include such a thing in the PR to be future proof?

@43081j
Copy link
Contributor

43081j commented Oct 3, 2018

I'll have a read soon.

One thing to keep in mind is that the legacy decorators babel produces are neither stage 0 nor stage 2, they are an incorrectly patched stage-0 by the looks of it:

https://github.com/babel/babel/blob/5150f5f5003b25d89f8b5041543412839cfde5fd/packages/babel-helpers/src/helpers.js#L999

They make use of PropertyDescriptor#initializer which doesn't seem to be a thing. New decorators (stage-2) do have an initializer but not on the descriptor (rather on the "class element descriptor", a new interface introduced by stage-2 decorators which wraps the property descriptor).

@mzeiher
Copy link
Author

mzeiher commented Oct 3, 2018

hehe that's the fun when using experimental features :)
that's the how the implementation works at the moment (still in development):
Stage-0 and Stage-0 babel case:
member:
if decorating a member which is a "value" property:
define "hidden" value-property with undefined as value (for ts and initializer for babel) on prototype (es extra in stage-2 case)
return get-set property with a delegate to the created "hidden" value property and then handles reflection/sync/re-render
if decorating a member which has get/set (case e.g. @prop() get whatever() {} or when another decorator already patched the property):
return a new get-set property which delegates to the previous get/set (in this case there is no initializer in babel case) and then handles reflection/sync/re-render

the same is done for stage-2 decorators, this way multiple decorators are possible which manipulates the property every decorator delegates to get/set to the previous get/set

for method decorators it's just the get/set case is used but the delegate goes to the value of the previous PropertyDescriptor which holds the original function or a overidden one from another decorator (value = function(...parameter) { original.value.apply(this, [...parameter] }

the delegation logic must be implemented from every decorator which patches a member or member-function so the one implementing the decorator must take care of that (if someone mixes the decorators created by you and for example own logging decorators), I'll add an example and a test for it...

edit:
TS definitly does not support multiple decorators, babel legacy and stage-2 work :) but it's a lot to write: https://github.com/mzeiher/ce-decorators/blob/master/src/log.stage2.ts a helper would be nice :)

@mzeiher
Copy link
Author

mzeiher commented Feb 17, 2019

cosing... implemented by the lit-element team

@mzeiher mzeiher closed this Feb 17, 2019
@mzeiher mzeiher deleted the babel-decorator-fix branch February 17, 2019 11:12
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.

4 participants