ember-cli / ember-template-imports

Template import support in Ember!
MIT License
74 stars 39 forks source link

Issue with declare class fields #242

Open BwehaaFox opened 2 months ago

BwehaaFox commented 2 months ago

I'm not entirely sure that the problem is related to this addon specifically, but it is clearly related to the *.gts. files

I have a situation where I need to extend a component from an addon. The addon class has a field interactive and I need to check this field to be true before making any further decisions in the code.

I had this working piece of code in a *.ts file and decided to move it to *.gts.

import SomeAddonComponent from 'some-addon/components/some-component';
import layout from 'some-addon/templates/components/some-component';

class MyComponent extends SomeAddonComponent {
  declare interactive: boolean;
  someFunction() {
    if (this.interactive)
      ...
    super.someFunction(event);
  }
}

export default setComponentTemplate(layout, MyComponent);

declare interactive is used because the linter complains about the absence of the this.interactive field, in order to actually declare the existence of the field forcibly

After changing the extension to *.gts the code stopped working correctly. As a result of debugging, I noticed that this.interactive contains undefined instead of a boolean value. Looking at the prototype of the object, I noticed that the class actually overrides the interactive field, although the *.ts files do not do this.

I perceived declare as part of typescript, so I can’t fully understand, am I using things for other purposes, or is there some kind of conflict with the *.gts files?

NullVoxPopuli commented 2 months ago

what is supposed to be setting this.interactive?

I made a little demo here of the underlying transform: https://stackblitz.com/edit/stackblitz-starters-nmegkz?description=Starter%20project%20for%20Node.js,%20a%20JavaScript%20runtime%20built%20on%20Chrome%27s%20V8%20JavaScript%20engine&file=index.mjs&title=node.new%20Starter

and the output is:

// ❯ node ./index.mjs
import SomeAddonComponent from 'some-addon/components/some-component';
import layout from 'some-addon/templates/components/some-component';
let MyComponent = class MyComponent extends SomeAddonComponent {
    interactive: boolean;
    // ^ of note, `declare` has been dropped
    someFunction() {
        if (this.interactive) {}
        super.someFunction(event);
    }
};
export default setComponentTemplate(layout, MyComponent);

but, in both cases, I would expect something to set interactive -- how does that happen?

NullVoxPopuli commented 2 months ago

in any case, I opened https://github.com/embroider-build/content-tag/issues/77

BwehaaFox commented 2 months ago

In fact, SomeAddonComponent looks like this:

export DEFAULT from './some-addon-default-config'

class SomeAddonComponent {
  get interactive() {
    return DEFAULT.interactive; // Default value - true
  }
  ...
}

MyComponent extends from it

class MyComponent extends SomeAddonComponent {
  declare interactive: boolean;
  someFunction() {
    if (this.interactive)
      ...
    super.someFunction(event);
  }
}

That is, this.interactive should be true, taken from get interactive() of the inherited class, and declare interactive: boolean; should be omitted altogether, as is, as I understand it, what happens in regular *.ts. But by ignoring declare, it ends up being perceived as a field override.

NullVoxPopuli commented 2 months ago

since it's defined in your parent class, TS shouldn't require the declare at all. You could be unblocked if the line is deleted altogether.

However, I do think this is probably a bug here: https://github.com/embroider-build/content-tag/issues/77 (idk if you want to poke at a PR for that :sweat: )

BwehaaFox commented 2 months ago

TS shouldn't require the declare at all

Perhaps the problem is that I'm importing js and not ts.

What I'm actually trying to do is extend this component, if this can help in any way

idk if you want to poke at a PR for that

Аs I see, the source code is in Rust, but unfortunately I’m not familiar with it

NullVoxPopuli commented 2 months ago

Perhaps the problem is that I'm importing js and not ts extend this component, if this can help in any way

You could write a dts file give that component a type?

but unfortunately I’m not familiar with it

No worries!

BwehaaFox commented 2 months ago

You could write a dts file give that component a type?

In general I can, but in this case I am more concerned about declare, since it is simply more convenient in terms of readability for a small component extension, when you only need to declare 1 field within the business logic. One way or another, linter notifications in this case do not interfere, but rather undermine the sense of perfectionism :)

If someone manages to solve this problem, I will be glad that other developers will not have to deal with this

I would rather take a PR for this addon to make it typed out of the box, and I am concerned that it uses the old file structure, which, according to my observations, causes problems with its import in gts, for which I had to make a component wrapper with the import of component and layer via setComponentTemplate as described above, since the component seemed not to see the template. So, if I may ask, I would like to know if users of the addon will have compatibility issues if the structure is changed from component/template to component/index under the current addon requirements for Ember CLI v3.13 or above?

NullVoxPopuli commented 2 months ago

I am concerned that it uses the old file structure

it's also deprecated for removal in ember-source v6.0 https://deprecations.emberjs.com/v5.x#toc_component-template-resolving

, I would like to know if users of the addon will have compatibility issues if the structure is changed from component/template to component/index under the current addon requirements for Ember CLI v3.13 or above?

I'm not 100% on when exactly component co-location was introduced (like if it was earlier or if 3.13 is the minimum), but if you only support ember-cli/ember-source versions that also support component co-location -- there would be no issue in the internal migration of the addon. (though, the migration would be addon/templates/components/foo.hbs to addon/components/foo.hbs (or index if you use component-structure=nested -- e.g. both addon/components/foo.js and addon/components/foo.hbs would be under addon/components/foo/index.js or addon/components/foo/index.hbs (sorry for verbosity if this is obvious!))

BwehaaFox commented 2 months ago

I managed to find RFCS, and it looks like this is good news for that addon. Release Versions: ember-source: v3.13.0

Thanks for the tip :3