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

Dynamic Selector Resolution #88

Closed
Viijay-Kr opened this issue Mar 1, 2023 · 7 comments
Closed

Dynamic Selector Resolution #88

Viijay-Kr opened this issue Mar 1, 2023 · 7 comments
Assignees
Labels
definitions issues related to definitions diagnostics issues related to the Diagnostics API enhancement New feature or request hover issues related to hover provider parser

Comments

@Viijay-Kr
Copy link
Owner

Is your feature request related to a problem? Please describe.
As mentioned in #86 dynamic selectors are not supported by the extension and were considered for future releases but eventually marked as trivial since there were no proper brainstorming or ideas.

dynamic selectors usually are common in UI libraries , Component frameworks etc. It's not a bad feature to atleast brainstorm.

Describe the solution you'd like
First solution mentioned in #86 can be elaborated as follows.
When accessing a dynamic selector one of the following patterns is probable to occur

At least In Typescript/Typescript react

Dynamic selectors in typescript can be accessed through the following type of syntaxes

  1. String Literal
    • concatenated string with a run time value . styles["mx_w"+"_"+width] -> can be evaluated to styles["max_w_320"] for instance
  2. Template Literal
    • The same principle of string literal but a using template literal syntax . styles[max_w_${width}]
  3. Identifier
    • Classic approach styles[width] -> width can be a prop that can hold values in the form of string or a named union.
    • in the case of string , computing value from inside the extension context will be challenging / no op.
    • if its a typed union , extension context can leverage with the typescript server and find out the possibles values using a typescript plugin at a given position for a given identifier.
    • worst case if the plugin doesn't work , babel parser could also be used but walking the tree and finding the node might be tricky.

After determining the type of dynamic selector , not all providers can be give useful intellisense.
The possible providers are

  1. Definition Provider
  2. Hover Provider
  3. Diagnostics

Definition Provider.

Definition provider can provide multiple definitions for a given position in the document.
In the case of dynamic selectors each inferred value at the given position can be collected by the definition provider

for instance in the use case mentioned in #86 the size prop can hold three values.sm | md | lg.
Now if the classes that correspond to these values are defined in the accessing style module , then three definition references will be provided.

Hover Provider

Hover takes the same approach as definitions , but will combine all the three code blocks into one large hover content.

for instance

.sm {
  // some properties
}
.md {
 // some properties
}
.lg {
 // some properties
}

It can get messy if the possibilities are long.

Diagnostics

Diagnostics should warn about missing selectors, the same way it does for static selectors except the fact it should consider the inferred types.

If the types are not able to be inferred , then the accessor at the position should be ignored.

Describe alternatives you've considered
Treat dynamic selectors as trivial and not support them.

Additional context
The probable solutions mentioned here in this ticket and in #86 are only feasible if one of the two approaches are realistic to implement

  1. A typescript server plugin . Should be the most viable option.
  2. Babel Parser . If possible then its easy to integrate since the whole extension is based on babel parser's AST
@Viijay-Kr Viijay-Kr added the enhancement New feature or request label Mar 1, 2023
@Viijay-Kr Viijay-Kr self-assigned this Mar 1, 2023
@Viijay-Kr Viijay-Kr added diagnostics issues related to the Diagnostics API definitions issues related to definitions hover issues related to hover provider parser labels Mar 1, 2023
@strlns
Copy link
Collaborator

strlns commented Mar 1, 2023

Great ideas!

I too think it's best to continue silently ignoring the dynamic string case.

Mainly because this is also the behavior of CSS modules at build time (with type checking): non-existing selectors are silently ignored, which is consistent with TS behavior.

Dynamic property access using strings silently returns undefined, except maybe for TS template string types or cases where as const is used.

After determining the type of dynamic selector , not all providers can be give useful intellisense.
The possible providers are

Definition Provider
Hover Provider
Diagnostics

I think I'd expect the extension to warn about non-existing selectors if the dynamic selector consists of a single identifier, but suggesting identifiers in scope that resolve to a valid selector probably isn't needed and might even be hard to understand.

I'm not sure if I will be able to provide a useful PR for this soon, but I really like the idea to validate statically analyzable dynamic selectors instead of just ignoring, given they only consist of a single identifier.

Again, this would be somewhat consistent with TS behavior for regular objects, although of course this extension cannot modify behavior of actual build setups using CSS modules.

@Viijay-Kr
Copy link
Owner Author

Thanks for the valuable insights.

I'm not sure if I will be able to provide a useful PR for this soon, but I really like the idea to validate statically analyzable dynamic selectors instead of just ignoring, given they only consist of a single identifier.

No worries . I will try to find some time for this soon.

@strlns
Copy link
Collaborator

strlns commented Mar 10, 2023

As mentioned in #92, this works pretty well in the typescript-plugin-css-modules TS server plugin.
In a small Vite project, even with PostCSS and nested selectors.

For example, here the class g-0 is missing in the CSS module file (which uses nested selectors via postcss-preset-env).

grafik

grafik

Unfortunately it seems like this approach is hard to integrate with the refactoring actions that this extension provides.

@Viijay-Kr
Copy link
Owner Author

@strlns I guess, this feature won't be possible without interacting with the typescript language server and letting typescript language server take control over it.

This could be what the typescript css modules plugin in doing

@strlns
Copy link
Collaborator

strlns commented Mar 14, 2023

Yes I see. I'm not sure if I'm able to code that. If I am, I probably wouldn't do it better than that plugin and/or without breaking features that this extension provides.

I just tested what happens if I rename a class from the point of usage in a project with Vite, vite-plugin-ssr and typescript-plugin-css-modules. The result wasn't pretty, the changed class name was randomly inserted somewhere in the CSS module (this extension not being enabled).

Might be fundamentally difficult to provide both things at once (TypeScript IntelliSense for CSS modules and editor actions)

@Viijay-Kr
Copy link
Owner Author

Viijay-Kr commented Mar 15, 2023

Interesting.

To speak against the feature, I believe this feature is a little outside the context of this extension.
The goal of this extension was and still is to tie css modules with JSX and TSX.

I wasn't very convinced to introduce typescript language service after requested by @karlhorky in #70 , however it seemed like a nice addon to the extension.

However type checking dynamic selectors seems like a problem of different context.

I don't do a lot of dynamic selectors in my projects , I feel like its a bit cumbersome to tie selectors with type definitions. They do not give any clear idea to my colleague what the selector does on first glance.

That said, I guess we can treat this issue as a no op . WDYT ?

@strlns
Copy link
Collaborator

strlns commented Mar 15, 2023

I think you are right.

I'll take the liberty to close this then, hope you don't mind?

@strlns strlns closed this as not planned Won't fix, can't repro, duplicate, stale Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
definitions issues related to definitions diagnostics issues related to the Diagnostics API enhancement New feature or request hover issues related to hover provider parser
Projects
None yet
Development

No branches or pull requests

2 participants