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

Issue with declare class fields #242

Open
BwehaaFox opened this issue Jul 25, 2024 · 9 comments
Open

Issue with declare class fields #242

BwehaaFox opened this issue Jul 25, 2024 · 9 comments

Comments

@BwehaaFox
Copy link
Contributor

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
Copy link
Collaborator

NullVoxPopuli commented Jul 25, 2024

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
Copy link
Collaborator

in any case, I opened embroider-build/content-tag#77

@BwehaaFox
Copy link
Contributor Author

BwehaaFox commented Jul 25, 2024

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
Copy link
Collaborator

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: embroider-build/content-tag#77 (idk if you want to poke at a PR for that 😓 )

@BwehaaFox
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

BwehaaFox commented Jul 26, 2024

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

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

No branches or pull requests

2 participants