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

0.4.2 Generates exports that are not picked up by vscode autocomplete #63

Open
bali182 opened this issue Mar 15, 2019 · 8 comments
Open

Comments

@bali182
Copy link

bali182 commented Mar 15, 2019

I have been using 0.3.7 up until now, which generated typings like this:

export const firstStyle: string;
export const otherStyle: string;

Now I tried to update to 0.4.2 which generates typings like this:

declare const styles: {
  readonly "firstStyle": string;
  readonly "otherStyle": string;
};
export = styles;

Is there an option to generate the typings like the older version? The newer one's exports are not picked up by VSCode's autocomplete, which was the biggest benefit for me besides not messing up class names accidentally.

@Quramy
Copy link
Owner

Quramy commented Mar 16, 2019

@nanot1m Do you have any idea?

@nanot1m
Copy link

nanot1m commented Mar 16, 2019

@bali182, can you provide some code example? and what typescript version are you using?

I could not reproduce :(
Снимок экрана 2019-03-16 в 13 59 23

@bali182
Copy link
Author

bali182 commented Mar 25, 2019

Here's a small demo of what I mean:
repro

@rfgamaral
Copy link

@bali182

That's because in the old format each class name was exported individually and TypeScript picked them up for auto import. As for auto complete, it's working fine for both formats as you can clearly see on your demo.

This could be somewhat solved by merging #66 (I've manually changed the .d.ts file for this demo):

image

However, the current state of #66 will give us another problem:

image

I've simulated that change with multiple .css files on my code base and since all of them export a default styles module, VS Code is picking all those files and in the example above you can see it's suggesting me 3 different styles to import from. Imagine if you have hundreds of deceleration files like this.

What needs to be done?

Merge #66 but not before fixing the exported named which is basically the feature request on #27.

@bali182
Copy link
Author

bali182 commented Jul 8, 2019

@rfgamaral I might have chosen the wrong words when saying autocomplete is not working.

From a developers point of view these are the 2 options:

1.) (Old) Start typing a class name, press Ctrl+Space, and see the classname immediately popping up
2.) (New) Manually type out import styles from '...', then on the imported object be able to access class names. When trying to import styles using said autocomplete, seeing all the exported styles in the project, and hoping your IDE sorts them nicely so the one you want is on the top.

In my opinion the second one is a step back for development speed. I'm not saying people might not like the second one, the purpose of this issue was to check if there is an option to generate the styles the old way (named exports) and if there isn't then ask for an option.

@rfgamaral
Copy link

What I'm trying to say is that your option 1 is more problematic than you think.

Imagine this:

file1.css

.myClass {}

file2.css

.myClass {}

file3.css

.myClass {}

Now you start typing myC... and press Ctrl+Space... That will give you 3 myClass references from all those 3 different files, which is the correct one? You might not notice and assume it's the first one, press Return and auto-import will import the wrong file. Now imagine you have more than 3 files with the same class name on each one of them. And since this is about CSS Modules you're more than likely to have the same class name across multiple files.

IMO, the second option is not a step back, it's the way it should be and it's not even complete because we still have a similar problem since all those files are named styles when they should be based on the original .css file. That's a different issue but if this is fixed you wouldn't have to manually type import styles... you'd just have to start typing the name of the CSS file, press . , auto-import would kick-in and import the correct file for you; continue typing and you get the correct class names you are expecting. Wouldn't that work for you?

@bali182
Copy link
Author

bali182 commented Jul 8, 2019

Right, the issue you mention is absolutely valid, we ran into it a few times as well. However in my opinion it's something that can be easily avoided with a bit of care when naming classes, and I heavily prefer named exports.

My point is, that these are all viable solutions. But would you be open for giving the user of the library the option to choose the right one for the project?

I'm not familiar with the structure of this project, but I'd be happy to implement it & submit a PR. If you don't want this in the library, it's not a big deal either, we'll just stick to 0.3.7, it works perfectly well.

@rfgamaral
Copy link

However in my opinion it's something that can be easily avoided with a bit of care when naming classes(...)

Yes, it can, but should you? From my point of view, that kinda defeats the purpose of CSS Modules. Why would you rather put extra effort into naming classes (one of two hardest things in programming 😆) instead of simply typing the name of the component you're working on followed by .?

My point is, that these are all viable solutions.

Sure, but not everything is a best practice.

But would you be open for giving the user of the library the option to choose the right one for the project?

Sorry if I passed the wrong impression but I don't own this project and that's not my decision to make. I'm just giving my opinion on the matter.

If you don't want this in the library, it's not a big deal either, we'll just stick to 0.3.7, it works perfectly well.

Please don't take this the wrong way but I honestly think you're trying to solve the wrong problem. There's a lot of open issues on this project and I'm sure some improvements will come in future versions, and if the option you're requesting will ever be implemented on this project, I can't tell, it's no up to me. But I wouldn't want to be stuck to such old version.

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

4 participants