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

TypeScript compilation error of attributeChangedCallback for Polymer.Element #5087

Closed
6 tasks done
rhysd opened this issue Feb 2, 2018 · 6 comments
Closed
6 tasks done

Comments

@rhysd
Copy link

rhysd commented Feb 2, 2018

Description

I created my Polymer element as following with TypeScript:

/// <reference path="./bower_components/polymer/types/polymer-element.d.ts"/>

class MyElement extends Polymer.Element {
    static get is() {
        return 'my-element';
    }

    static get properties() {
        return {};
    }
}

And I added a callback method to be notified the attribute changes:

attributeChangedCallback(name: string, oldVal: string | null, newVal: string | null) {
    super.attributeChangedCallback(name, oldVal, newVal);
}

But tsc compiler raised an error as following:

index.ts:13:15 - error TS2339: Property 'attributeChangedCallback' does not exist on type 'Element'.

If my understanding about the Polymer elements' lifecycle is correct, a callback method in super instance must be called to handle lifecycle hooks properly. But tsc raises an error for it.

https://www.polymer-project.org/2.0/docs/devguide/custom-elements

Live Demo

https://github.com/rhysd/typescript-polymer-bug

Steps to Reproduce

  1. $ git clone https://github.com/rhysd/typescript-polymer-bug.git
  2. $ cd typescript-polymer-bug
  3. $ npm start

Expected Results

Compilation is successfully done

Actual Results

Compilation error:

index.ts:13:15 - error TS2339: Property 'attributeChangedCallback' does not exist on type 'Element'.

13         super.attributeChangedCallback(name, oldVal, newVal);
                 ~~~~~~~~~~~~~~~~~~~~~~~~


Browsers Affected

  • Chrome
  • Firefox
  • Edge
  • Safari 9
  • Safari 8
  • IE 11

Versions

  • Polymer: v2.4.0
  • webcomponents: v1.1.0
  • TypeScript 2.7
@TimvdLippe
Copy link
Contributor

Hm it is defined in the properties-changed mixin (

/**
* Implements native Custom Elements `attributeChangedCallback` to
* set an attribute value to a property via `_attributeToProperty`.
*
* @param name Name of attribute that changed
* @param old Old attribute value
* @param value New attribute value
*/
attributeChangedCallback(name: string, old: string|null, value: string|null): void;
) which PropertyAccessors extends (
function PropertyAccessors<T extends new (...args: any[]) => {}>(base: T): T & PropertyAccessorsConstructor & Polymer.PropertiesChangedConstructor;
) which PropertyEffects extends (
function PropertyEffects<T extends new (...args: any[]) => {}>(base: T): T & PropertyEffectsConstructor & Polymer.TemplateStampConstructor & Polymer.PropertyAccessorsConstructor;
) which should then end up in Polymer.Element via
function ElementMixin<T extends new (...args: any[]) => {}>(base: T): T & ElementMixinConstructor & Polymer.PropertyEffectsConstructor & Polymer.PropertiesMixinConstructor;

I am not sure why this chain does not end up in your project. We probably have to update our own types to figure out where it is missing out on this property.

@rhysd
Copy link
Author

rhysd commented Feb 6, 2018

Yeah, I could find the constructor chain in my local directory also. And other callbacks such as connectedCallback are callbale.

@TimvdLippe
Copy link
Contributor

Actually, all methods defined in property-changed.d.ts are not compiling. This might be a TypeScript bug, as I can't spot anything different with this file compared to the other files we define. @aomarks do you maybe know why this file is not properly being picked up by TypeScript?

@aomarks
Copy link
Member

aomarks commented Feb 9, 2018

I see what's going on. The mixin declarations we generate are defined as a function that returns a constructor for 1) the passed base class 2) that mixin's interface and 3) additional mixins that get automatically applied as picked up by analyzer using the @appliesMixin annotation.

The problem is that those mixins from 3) that are applied transitively are not automatically lifted up. So unless the @appliesMixin annotation is manually added at the top level, any mixin that is applied transitively is not declared.

In the ElementMixin case, the tree looks like this:

- ElementMixin
  - PropertyEffects
    - TemplateStamp
    - PropertyAccessors
      - PropertiesChanged
  - Properties
    - PropertiesChanged

Which means Polymer.Element will correctly extend ElementMixin and its direct @appliesMixins: PropertyEffects, and Properties. But the transitive ones TemplateStamp, PropertyAccessors, and PropertiesChanged are not declared.

Options:

  1. Manually add @appliesMixin annotations to ElementMixin.
  2. Update Analyzer to traverse the @appliesMixin tree and flatten it out.
  3. Update the TypeScript generator to do 2).

@rictic
Copy link
Contributor

rictic commented Feb 9, 2018

I definitely think we should not do option 1, and should do either option 2 or 3. I suspect that option 3 is the better one, because it seems like it might be better able to handle stuff like DeduplicatingMixin, but if @justinfagnani has thoughts on that I'd defer to him here.

@justinfagnani
Copy link
Contributor

3 sounds good to me. The analyzer returns sufficient information - it could certainly flatten as a convenience, but it's not necessary.

aomarks added a commit to Polymer/gen-typescript-declarations that referenced this issue Feb 13, 2018
…vely.

Previously, only the immediately applied mixins were accounted for, but
not ones that were applied transitively.

Fixes Polymer/polymer#5087
aomarks added a commit to Polymer/gen-typescript-declarations that referenced this issue Feb 13, 2018
…vely. (#91)

Previously, only the immediately applied mixins were accounted for, but
not ones that were applied transitively.

Fixes Polymer/polymer#5087
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

5 participants