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

Add Vue support #171

Closed
wants to merge 1 commit into from
Closed

Add Vue support #171

wants to merge 1 commit into from

Conversation

Timmmm
Copy link

@Timmmm Timmmm commented Oct 20, 2020

This also adds a test that may or may not test anything.

Fixes #162

This also adds a test that may or may not test anything.
Copy link
Collaborator

@mrseanryan mrseanryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work - a few TODOs:


  • CHANGELOG

Please add an 'Added' entry to the top of the CHANGELOG.md file

"strictNullChecks": true
},
"include": ["./**/*.ts", "./**/*.vue"],
"atom": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove atom section

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah not sure why this is in here - I just copied it from another example, you might want to delete it there too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, but we can start by removing it here ...

Copy link
Collaborator

@mrseanryan mrseanryan Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logged #172 but in this MR we can at least avoid adding more such files with these small issues.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#172 has been fixed

{
extension: 'vue',
isMixedContent: true,
scriptKind: ts.ScriptKind.Deferred,
Copy link
Collaborator

@mrseanryan mrseanryan Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder could this cause any issues.

For example if an existing user has some vue files that they do NOT want scanned.

Also, to be honest I do not fully understand these parameters 😀

We could hide this behind a new option like --vue - then that could also auto-suppress the shims file shims-tsx-d-ts

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, the README change is much smaller (just document the new option)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the approach of adding a new option --vue

@pzavolinsky what do you reckon ?

@mrseanryan
Copy link
Collaborator

mrseanryan commented Oct 20, 2020

(related)

  • @mrseanryan check what happens if more than 3 issues found -> itest should fail ...

@Timmmm about the itest not failing:

The itests use the exitWithCount option:

Set the process exit code to be the count of files that have unused exports.

So it is not counting the number of unused items - it is counting the files.

When I tried it on this branch, it worked ok (by making a copy of Math.ts)

For future - Logged #172 to improve how itests checked.

@@ -0,0 +1,6 @@
export function add1(x: number) { return x + 1; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename add1 to unused

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions were just copied from another example, I left the names for consistency.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in master - if you rebase you will see.

export function add1(x: number) { return x + 1; }

// ts-unused-exports:disable-next-line
export function add2(x: number) { return x + 2; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename add2 to unusedButDisabled

@mrseanryan
Copy link
Collaborator

mrseanryan commented Oct 21, 2020

[UPDATED - itests]

  • add itest with more complex embedded Typescript.

  • Mixins - standard part of TypeScript, so not needed in this PR

  • @Timmmm - are there any other Vue features we need to think about?

{
extension: 'vue',
isMixedContent: true,
scriptKind: ts.ScriptKind.Deferred,
Copy link
Collaborator

@mrseanryan mrseanryan Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comment to explain the values used for:

isMixedContent,
scriptKind

Copy link
Collaborator

@mrseanryan mrseanryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments needed to explain parameters chosen (see above)

@mrseanryan
Copy link
Collaborator

mrseanryan commented Oct 21, 2020

  • add a new unit test file

The PR has an itest, but some unit tests are needed for future maintenance and debugging.

Add a new file like: vue.feature

Example:

https://github.com/pzavolinsky/ts-unused-exports/blob/master/features/base-url-undefined.feature

@Timmmm
Copy link
Author

Timmmm commented Oct 21, 2020

what about Mixins? Looks like something that could require special support.

I think mixins are a standard Typescript feature.

@Timmmm Timmmm closed this Aug 8, 2024
@FlorianWendelborn
Copy link

Since this is closed now, in case someone is still looking for a Vue compatible alternative you may want to try knip

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

Successfully merging this pull request may close these issues.

Vue support
3 participants