Improve types for properties of <style> as a Tag Variable
#2776
Conversation
I’m reasonably confident that this **works** (i.e. I can observe that it addresses the issue I want to address.) I’ll address some things I’m less confident in PR notes.
|
The committers listed above are authorized under a signed CLA. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2776 +/- ##
=======================================
Coverage 87.98% 87.98%
=======================================
Files 363 363
Lines 44537 44537
Branches 3436 3436
=======================================
Hits 39187 39187
Misses 5326 5326
Partials 24 24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| input: Input; | ||
| return: { value: () => Return }; | ||
| return: { | ||
| value: (() => Return) & Value; |
There was a problem hiding this comment.
So I came back and checked out the actual runtime behavior and it appears that <style/foo> produces a module namespace object. In hindsight, this isn't surprising at all. That's exactly what the docs say:
If the <style> tag has a Tag Variable, it leverages CSS Modules to expose its classes as an object.
Which suggests that the following should be mutually exclusive:
<style/> /* () => HTMLStyleElement */
<style/binding/> /* Record<string, string> */And even that isn't quite right. The runtime representation of CSS modules may be tooling-dependent. I'm struggling to find this now in the docs, but unless I'm mistaken, I remember reading that this is deferred to the bundler. In any case, what I see with my (mostly) out-of-the-box project (Marko 6 + Marko Run + Vite) is an interface more like:
interface CSSModule {
[key: string]: string;
default: Record<string, string>;
}(That's not actually a valid type definition, but hopefully it gets the point across.)
Anyway... all of this is a long way around the barn to say:
- I'm even more doubtful that these types are right.
- I'm actually kind of doubtful that static type definitions are even the right place to solve this!
On Discord, @DylanPiercey also mentioned interest in special casing CSS module typegen in the Marko language server. That sounds like it would produce more appealing types in the first place... it would certainly be better than this change in its current state.
I'll defer to the Marko team on whether this PR is worth pursuing any further. But it does feel pretty iffy to me, and I won't mind at all if y'all close it for now in lieu of a more robust/fitting solution.
|
Another observation: before I explored the solution in this PR, I had originally tried just augmenting the types locally. I found it impossible to define a more targeted augmentation because it would conflict with the underlying types, and the language server just disregarded my augmentation. That said, today I happened to discover a hacky way to do pretty much what I tried the first time around. If I add <return=({} as Record<string, string>)/>... As a workaround, I think this is sufficient for what I'm trying to achieve right now. It may also be worthwhile if anyone else is looking for a quick way to do away with this particular case of red squigglies in their editor. And maybe that also adds more weight to the approach in this PR not being the best solution for now? Footnotes
|
|
Stuff like this needs to work too <div class=css.foo>TEST</div>
<style/css>
.foo {
background-color: red;
}
</style>But currently it says that With regards to special casing on css modules in lang server, the output depends on the bundler, for example in vite it is possible to configure so classes like |
Fixes #2775, eliminating TypeScript type errors when accessing properties associated with CSS-defined classes.
Description
As described in #2775, this is an attempt to produce the pragmatically expected behavior. I.e. given the same example snippet from the docs/inlined there:
Before:
() => HTMLStyleElementAfter:
(() => HTMLStyleElement) & Record<string, string>stringI’m reasonably confident that this works (i.e. I can observe that it addresses the issue I want to address.) Some things I'm less confident about:
I have some doubts about whether this is the right way to make type-level changes in the project structure generally. I originally asked about this on Discord, but decided to just go ahead and open a PR so there's a more concrete place for feedback.@DylanPiercey I think the matching changes should be right based on your Discord response, let me know if I'm mistaken!I'm not sure if this approach and the resulting type is actually expected/right! As mentioned in
<style>as Tag Variable lacks types for class properties #2775, I'm not sure if the type actually does match the runtime. I'm happy to check, but I'm running out of energy for dev stuff today and wanted to get at least this much open for feedback before I wrap up.I'm not thrilled with the more global change: introducing
& anyto the corresponding types for unspecified tags. I'd be happy to address this (e.g. with a conditional type or similar mechanism), but I didn't want to get too bogged down in that until the previous point is addressed.Checklist:
Re: documentation, I'm open to direction!
Re: tests, as I've been writing this up @DylanPiercey mentioned on Discord type-level testing in https://github.com/marko-js/language-server. I'd be happy to look into that, possibly tomorrow, if it would be helpful!