Improve types for properties of `<style>` as a Tag Variable
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:
<style/styles>
/* ^ [1] */
.foo {
border: 1px solid red;
}
</style>
<div class=styles.foo/>
/* ^ [2] */
Before:
() => HTMLStyleElement- Error: Property 'foo' does not exist on type '() => HTMLStyleElement'
After:
(() => HTMLStyleElement) & Record<string, string>- Type:
string
I’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 #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:
- [x] I have read the CONTRIBUTING document and have signed (or will sign) the CLA.
- [ ] I have updated/added documentation affected by my changes.
- [ ] I have added tests to cover my changes.
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!
⚠️ No Changeset found
Latest commit: dfad3f825aac01ba9709f2b4813aa12720255120
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 87.98%. Comparing base (34b88ac) to head (dfad3f8).
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
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 src/tags/style.marko with...
<return=({} as Record<string, string>)/>
... ~~so long as I don't import that tag~~[^1] this seems to produce the same approximately-desired effect (at least in VSCode).
[^1]: Apparently this isn't an issue either, presumably because <style> is special cased as a builtin: it just shows up as unused, rather than shadowing the builtin as I'd expected.
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?
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 variable 'css' used before its declaration., so for that lsp server needs to be modified anyways.
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 .foo-bar become fooBar in javascript or "foo-bar" or both, so that would need to be handled somehow too.
Emitting .d.ts from the vite plugin or better some json file about the modules configuration for the lang server to read would be a solution.
Also it would be possible to intersect concrete class types with Record<string, string> so if it breaks it won't cause problems but users will still get autocomplete if it works,
as an in the above example css: Record<string, string> & { foo: string },
but that will mean that incorrect class names will still typecheck.
Vue has support for inline css modules so would be worthwhile to look into that.