Polymer / vscode-plugin

Provides autocompletion, linting, and more for web components.
Other
74 stars 11 forks source link

Doesn't jump to definition if using computed methods via Typescript with target ES5. #49

Open bahrus opened 7 years ago

bahrus commented 7 years ago

Update: The description below was too broad. See below for more accurate description. It was a specific Typescript use case that was causing this issue.


With the windows version 0.4.0, I'm finding that I can navigate from a Polymer 1 custom element tag to the code definition, only if the main code definition ( Polymer({is: 'my-custom-element'}) ) is inline in the element tag html file. If the definition comes from a , it doesn't work.

Personally, I'm on the fence on if I should continue to do this anyway. I tend to use script references because I like to use TypeScript. On the other hand, it seems more convenient for consumers not to have to jump around so much, especially to see the core definition (properties / methods).

So I would really like to know if this is a feature you plan to support. If so, and you expect that feature to be working already, I can provide further details (possibly the issue is the way I'm using TypeScript, and the way the file compiles). If not, I will adjust my definitions.

rictic commented 7 years ago

We do expect that to work, and it's a bug if it doesn't. I'm out for the next few days but I'll see if I can take a look. A couple things to debug, try running polymer analyze and seeing if the element is found and if it has a source range. Also see if you can come up with a simple set of code to reproduce the issue.

rictic commented 7 years ago

Hm, I've just tried this scenario out and I was able to jump to the definition of an element defined in a JS file that was imported using <script src='./element.js'></script>.

Is your code open source, or can you reduce it down to a test case that's sharable?

bahrus commented 7 years ago

It is open source. I'm trying to get:

https://github.com/bahrus/crystal/blob/master/testXSlickGrid.html#L40

to jump to:

https://github.com/bahrus/crystal/blob/master/xtal-elements/x-slick-grid/x-slick-grid.js#L68

I confirmed that if I reduce the file to a very simple Polymer one liner, it works. I think the weird thing typescript is doing with _a, which might be a symptom of something weird I'm doing, may be throwing off the polymer analyzer.

bahrus commented 7 years ago

I've renamed the issue, so it's clear this appears to be a pretty obscure "bug."

In order to add compile type linkage of sorts between observers and method names, one can use computed method names, as shown in this Typescript code:

const onPropChange = 'onPropChangeString';
Polymer({
    is: 'x-y-z',
    properties:{
        prop:{
            type: String,
            observer: onPropChange,
        }
    },
    [onPropChange] : function(newVal){
        console.log(newVal);
    }

})

This results in the following JavaScript being emitted:

var onPropChange = 'onPropChangeString';
Polymer((_a = {
        is: 'x-y-z',
        properties: {
            prop: {
                type: String,
                observer: onPropChange,
            }
        }
    },
    _a[onPropChange] = function (newVal) {
        console.log(newVal);
    },
    _a));
var _a;

This appears to block the Polymer Analyzer from identifying this as a Polymer element.

rictic commented 7 years ago

Ah, tricky tricky. I see three options in the short term (long term we would like to add first class typescript support):

  1. teach the analyzer to recognize that style of expression. it's not so bad for us to tell that the object literal being assigned to _a is what we should treat as the polymer declaration object, but it would be more work to work out that it will have a method named onPropChange, as the string is stored in a var in the ES5, not a const
  2. have typescript emit ES6 during development, and go to ES5 only when building for production
  3. don't use computed property names in your typescript, and lean instead on the upcoming lint rule https://github.com/Polymer/polymer-linter/issues/29 to give you assurance that your observers map to functions on the element

What do you think @bahrus?

I've also filed https://github.com/Polymer/polymer-analyzer/issues/508 to make it easier to find this class of issue.

bahrus commented 7 years ago

Yes, makes sense. Feel free to close this issue.