Also allow using the modifier on SVG elements (types only)

[boris-petrov]

Sorry, one more change. 😄

The lint:types task fails locally on my side (unrelated to this change). Not sure what I'm doing wrong but I hope here it will be fine.

cc @jelhan

[jelhan]

@bwbuchanan proposed in #235 to fix the issue using ElementCSSInlineStyle. Did you tested that?

Suggested change

	Element: HTMLElement | SVGElement;
	Element: ElementCSSInlineStyle;

[boris-petrov]

Yes, that also works, I've committed it.

[boris-petrov]

Nope, actually it doesn't work, you can see the error in the CI. Ideas?

[jelhan]

This seems to be a limitation of the types provided by ember-modifier. I reproduced in TypeScript playground: https://www.typescriptlang.org/play?#code/MYGwhgzhAEBCkFNoG8BQ0PWAewHYQBcAnAV2AOyIAoEQAuaAURAQFsFcCBKFAX1X6pQkGAGEAFgEsQAE2gIAHgQ4yY8CEjSYseQqXKUa9Ji3adRAZQsBJXCEm4EFggE8WPLdswQSABwTUtFwA3OiY-LxAA

I think we should rollback to your original solution HTMLElement | SVGElement. Or do you now how to workaround it, @bwbuchanan?

[bwbuchanan]

This is what I used in types/ember-style-modifier/index.d.ts:

import type { ModifierLike } from '@glint/template';

type StyleModifier = ModifierLike<{
  Args: {
    Positional: [styles: Record<string, string>] | [];
    Named: Record<string, string>;
  };
  Element: ElementCSSInlineStyle;
}>;

const style: StyleModifier;

export default style;

[bwbuchanan]

Does it work if using the type ElementCSSInlineStyle & Element?

I think HTMLElement | SVGElement is too specific. It excludes MathMLElement, for example.

[boris-petrov]

@bwbuchanan this seems to work, yes. But... is it really better? Why do you prefer/need ElementCSSInlineStyle over HTMLElement | SVGElement? I definitely think the latter is more readable/understandable.

[jelhan]

This is what I used in types/ember-style-modifier/index.d.ts:

import type { ModifierLike } from '@glint/template';

type StyleModifier = ModifierLike<{
  Args: {
    Positional: [styles: Record<string, string>] | [];
    Named: Record<string, string>;
  };
  Element: ElementCSSInlineStyle;
}>;

const style: StyleModifier;

export default style;

I think that does not check if the interface is compatible with ember-modifier's interface. Therefore not running into this bug.

[bwbuchanan]

I think that does not check if the interface is compatible with ember-modifier's interface. Therefore not running into this bug.

Yes, you are correct.

I think that the type ElementCSSInlineStyle & Element should correctly capture all elements that support CSS inline styles, and also conform to ember-modifier's interface.

At the moment, this is basically the same as HTMLElement | SVGElement | MathMLElement, but the latter type is too specific and would not be future-proof.

[jelhan]

Let's do the following:

1. Land this with HTMLElement | SVGElement | MathMLElement.
2. Explore if we could fix types in ember-modifier to allow ElementCSSInlineStyle as a follow-up.

@boris-petrov: If you have capacities adding a test for MathMLElement would be great. But I'm okay merging without that test.

@bwbuchanan: Thanks for your input. Highly appreciated!

[jelhan]

Suggested change

	Element: ElementCSSInlineStyle & Element;
	Element: HTMLElement | SVGElement | MathMLElement;

[jelhan]

I just looked more deeply into intersection types. ElementCSSInlineStyle & Element seems to be a valid approach as well. @boris-petrov: I keep it up to you, which one we are going with.

[boris-petrov]

I added a simple test for a math element - hopefully that tests this case. However, as you can see, it fails... 😄

Type 'Element' is missing the following properties from type 'ElementCSSInlineStyle': attributeStyleMap, style

Obviously math elements are treated as Element. Even cases like <math><mtext {{style display="none"}}>test</mtext></math> don't work. Not sure why... ideas?

Otherwise I left it using ElementCSSInlineStyle as @bwbuchanan is right that this is more future-proof.

[boris-petrov]

Perhaps we can remove the Element at all? I think pretty much every element can be styled so...

[jelhan]

I must admit that I'm not an expert on that topic. But for me it looks like a bug in upstream typing of the MathMLElement. I tend towards landing it with Element. We can still explore if we can provide more detailed types in a follow-up.

[bwbuchanan]

I think this is a glint bug, typing <math> as Element instead of MathMLElement.

See: https://github.com/typed-ember/glint/blob/main/packages/template/-private/dsl/types.d.ts

[bwbuchanan]

I suggest that Element & ElementCSSInlineStyle is still the correct type for the element argument, but we'll need to disable the <math> test until glint is updated.

[jelhan]

I think this is a glint bug, typing <math> as Element instead of MathMLElement.

Are you sure that those types are defined by glint? I would expect glint to pull them in from a standard package.

I suggest that Element & ElementCSSInlineStyle is still the correct type for the element argument, but we'll need to disable the <math> test until glint is updated.

I prefer to use what allows all supported use cases for now. And make the types more strict in a follow-up as soon as we found a working solution.

[boris-petrov]

Sorry for the delay. I left the Element key and set it to just Element. I also added a TODO for future readers.

[boris-petrov]

Ah, no, Element: Element leads to Property 'style' does not exist on type 'Element'.. So it has to be without Element.

[boris-petrov]

I also tried using HTMLElement | SVGElement | MathMLElement but that also fails the math test. So the only way I managed to "fix" it was to remove Element at all.

[jelhan]

I wasn't able to reproduce the issues with <math> element types in TypeScript playground. So it might be related to Glint. I feel we need to debug it more in detail.

I would recommend keeping this one to SVG support only. We have a clear path forward for that case.

I don't feel comfortable removing the Element key all together. I think Element & ElementCSSInlineStyle is the correct type for it. Let's land the SVG fix with that.

Let's pull the test for <math> in a separate PR, mark it as WIP and solve it independently without blocking this fix.

[boris-petrov]

Done.

[jelhan]

Thanks a lot!

[jelhan]

Released as v4.3.0: https://github.com/jelhan/ember-style-modifier/releases/tag/v4.3.0

[jelhan]

I think this is a glint bug, typing <math> as Element instead of MathMLElement.

See: https://github.com/typed-ember/glint/blob/main/packages/template/-private/dsl/types.d.ts

@bwbuchanan Did you reported that bug to Glint?

[jelhan]

I think this is a glint bug, typing <math> as Element instead of MathMLElement.
See: https://github.com/typed-ember/glint/blob/main/packages/template/-private/dsl/types.d.ts

@bwbuchanan Did you reported that bug to Glint?

I reported it upstream here: typed-ember/glint#758