search

daidodo / format-imports-vscode

Format imports and exports for JavaScript and TypeScript in VS Code.
https://marketplace.visualstudio.com/items?itemName=dozerg.tsimportsorter
MIT License
62 stars 5 forks source link

Prevent removal of "unused" twin.macro #12

Closed danielkcz closed 4 years ago

danielkcz commented 4 years ago

Is your feature request related to a problem? Please describe. The twin.macro is from a family of babel-plugin-macros for TailwindCSS framework. In practice, it might look like.

import tw from 'twin.macro'

export default function() {
  return <div tw="my-2" />
}

Unfortunately, that import is being removed as unused, but macros are a rather special beast similar to React.

Describe the solution you'd like It would be probably best to cover this in groupRules, some different flag that prevents the removal of that group. The React/h could be handled this way too so it doesn't need to be hardcoded.

Describe alternatives you've considered I tried the following, but it has no effect, and import is removed.

"tsImportSorter.configuration.groupRules": [
  {
    "flag": "all",
    "regex": "^twin.macro$"
  },
  "^react(-dom)?$",
  {},
  "^[@]",
  "^[.]"
]
daidodo commented 4 years ago

@FredyC, thanks for the feedback!

Workaround solution: You can add comment ts-import-sorter: disable to exclude it from formatting. E.g.:

import tw from 'twin.macro'  // ts-import-sorter: disable

export default function() {
  return <div tw="my-2" />
}

In my end, the issue is Typescript Compiler thinks tw is not used. If it's different in your VSCode, I'd like to see an example repo or how to setup so I can add this feature.

danielkcz commented 4 years ago

In my end, the issue is Typescript Compiler thinks tw is not used.

It is true, but I have disabled noUnusedLocals and I am using ESLint instead which can be configured with exceptions.

Thanks for the workaround, at least I don't have to disable a whole thing. So far this is the best plugin for import handling, so thank you for it.

daidodo commented 4 years ago

@FredyC, could you share how to setup the exception in ESLint?

danielkcz commented 4 years ago 
no-unused-vars: off
'@typescript-eslint/no-unused-vars': ['warn', { varsIgnorePattern: 'tw|css' }]
daidodo commented 4 years ago

@FredyC , thanks!

I'll close this issue if the workaround works. Feel free to open it or a new one if you have further questions.

danielkcz commented 4 years ago

@daidodo Well, I cannot open it once it's closed by maintainer :)

And the workaround is not really great option. It's always better to have less clutter in the code. I would rather have something outlined in OP with groupRules so it's more universal. There might be more libraries/macros out there that could be facing such an issue.

I might attempt at a PR if you are willing to give me some hints on where to start.

daidodo commented 4 years ago

@FredyC, I may need a definition of the problem (please be generic), and details of the solutions from you to start with. I'm totally fine with PRs if you are willing to contribute.

danielkcz commented 4 years ago

The problem There should be a way to configure extension with exceptions when not to remove unused import. The React/h shouldn't be only hard-coded options.

The solution(s) I don't know specifics how the removal of unused works. The easiest, in my opinion, is to have a different config value with a list of RegExp for import path and ignore those matching from removal.

I haven't fully understood how flag works in groupRules. The tw import should be considered non-script imports so by having flag === undefined it should accept it as a rule. However, I suppose that "remove unused" routine kicks in afterward and ruins it anyway. It would be probably best to have a different key than a flag so the sorting rule can be defined separately. Something like...

{
  "regex": "^twin.macro$",
  "keepAlive": true
},
danielkcz commented 4 years ago

I've glimpsed over the code and probably the best spot to handle this would be here...

https://github.com/daidodo/tsimportsorter/blob/a063e08fca3c163f4d5c92fe65b0e5cea10b3750/src/format/sort/index.ts#L23-L26

What remains is to decide on which one of solutions sounds better to you.

daidodo commented 4 years ago

@FredyC, here is how this extension removes unused names:

  1. When parsing the source code, gather all used names. (code).
  2. Use TS compiler to determine unused names. (code)
  3. Remove unused names based on the above info, in the code you posted, before grouping and sorting.

For a generic solution, here are a few options I can think of now:

Global config

Add a keepUnusedNames config:

"keepUnusedNames": { "paths": ["^twin.macro$"], "names": []}

You can specify paths, which applies to all names in an import from matched path, or names from any imports. This config applies to all groups.

Group config

Add a field in GroupRule to indicate this group/sub-group doesn't remove unused names. But the problems with this are: 1) You have to define a group/sub-group to just keep unused names; 2) There might be unexpected results if you set a wrong regex for the group.

The above solutions are based on the assumption that people may want to keep any names from any imports. But if there is a pattern/rule, there might be other options.

Please tell me your thoughts! Thanks!

danielkcz commented 4 years ago

I see a slight advantage with group config that you don't need to repeat the regex, but it's true that it might be different from what you want to sort.

So I yea, I would go with global config, although I would call it keepUnused since names is already a property inside.

Also, I think there should be the default for React/h that will be merged into that config value instead of hard coding it.

I will start working on PR tomorrow-ish unless you want to handle it yourself.

daidodo commented 4 years ago

This feature is for something not understandable by the TS compiler and h/React are handled already, so you don't need to be bothered by them. Please also add integration test examples under src/test/suite/examples. Sorry I didn't write a contribution guide so if you have any questions just ask me. And I will work out one soon, hopefully before your PR.

Thanks!

daidodo commented 4 years ago

After a few thoughts, I want to make some modifications to the original proposal and write down a specification to avoid confusions.

Use Cases

  1. I want to keep all unused names imported from some paths.
  2. I want to keep some unused names imported from some paths.

(I don't see a case to keep a name from all import paths. If you know such, please tell me).

keepUnused

keepUnused is an array, with each element describing what names from what paths will be kept, e.g.:

"keepUnused": [
    { "paths": "^twin[.]macro$", "names": ["^tw$"] },
    { "paths": "^@emotion/" },
    "^@another_macro"
]

You need to match both paths and names to keep an unused name. If names is null or empty, all names are kept. If paths is null, ignore it. If the element is a string APattern, it equals to { paths: "APattern" }.

Edge Cases

"keepUnused": [""]

This effectively keeps all unused names from all paths.

"keepUnused": [{ "paths": "", "names": ["Hi"] }]

This keeps Hi from all import paths, if there is really a need for it.

Edit

Merging

keepUnused from different config sources will be merged instead of overwritten, the same as exclude.

daidodo commented 4 years ago

@FredyC, I've published a Contribution Guide draft. Hope that can help you!

danielkcz commented 4 years ago

The guide looks fine, thanks. I am OS maintainer myself (mobx), so nothing new to me there :)

Some things to clear up.

daidodo commented 4 years ago

@FredyC, thanks for your suggestions! I'm ok with 1 and 3, but I still think names should be an array of regex patterns, because:

Cheers.

daidodo commented 4 years ago

@FredyC, please check v2.1.0 for the latest changes we made.

Thanks again for your contribution!

danielkcz commented 4 years ago

Cool, it's working nicely, thanks for fast cooperation on this. We have made the world a better place 😎