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.

Sign up for GitHub

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

Jump to bottom

Add UnoCSS plugin #638

Merged
merged 5 commits into from
May 18, 2024
Merged

Add UnoCSS plugin #638

merged 5 commits into from
May 18, 2024

Conversation

hyoban
Copy link
Contributor

@hyoban hyoban commented May 14, 2024

No description provided.

webpro
webpro reviewed May 15, 2024
View reviewed changes
Copy link
Collaborator

@webpro webpro left a comment

Choose a reason for hiding this comment

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

Thanks @hyoban! No modifications seem to have been made, any chance you could still customize or just remove things specifically tailored to UnoCSS?

@hyoban
Copy link
Contributor Author

hyoban commented May 15, 2024

Sorry. I don't quite understand what I need to modify, can you be more specific? I basically implemented the unocss plug-in according to the vite plug-in.

webpro
webpro reviewed May 15, 2024
View reviewed changes
packages/knip/src/plugins/unocss/types.ts Outdated
@@ -0,0 +1,4 @@
export type PluginConfig = {
plugins?: string[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This type doesn't seem correct?

packages/knip/src/plugins/unocss/types.ts Outdated
@@ -0,0 +1,4 @@
export type PluginConfig = {
plugins?: string[];
entryPathsOrPatterns?: string[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this type does not exist in UnoCSS config

packages/knip/src/plugins/unocss/index.ts Outdated

const entry: string[] = [];

const production: string[] = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove empty arrays

packages/knip/src/plugins/unocss/index.ts Outdated

const title = 'UnoCSS';

const enablers: EnablerPatterns = ['unocss', /^@unocss\//];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use only unocss and @unocss/cli for better precision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are other dependencies to consider, such as @unocss/webpack, @unocss/postcss

Copy link
Collaborator

Choose a reason for hiding this comment

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

The "enabling packages" should only be root/core modules. The rest is referenced elsewhere, in the tool configuration. The plugin returns those to Knip, and packages can then be reported as unlisted or unused.

Having more enablers is not wrong, I just think a smaller list is more precise/easier to understand for consumers. Otherwise we'd have to try and list all possible plugins/dependencies etc. in all plugins which doesn't really make sense imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My previous understanding was wrong, it seems we only need ['unocss']

packages/knip/fixtures/plugins/unocss/unocss.config.ts Outdated
import { defineConfig } from 'unocss'

export default defineConfig({
// ...UnoCSS options
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to use a more comprehensive config file here to make it more real-world.

@hyoban
Copy link
Contributor Author

hyoban commented May 16, 2024 •
edited
Loading

Thank you for your review. Sorry, I thought the default code generated from CLI could be left alone.

Another question, can the plug-in tell Knip to ignore some dependencies that it thinks do not exist when using UnoCSS? For example uno.css, virtual:uno.css, virtual:unocss-devtools

@webpro
Copy link
Collaborator

webpro commented May 18, 2024 •
edited
Loading

Thank you for your review. Sorry, I thought the default code generated from CLI could be left alone.

No worries! Thanks for contributing to Knip, I really appreciate it.

Another question, can the plug-in tell Knip to ignore some dependencies that it thinks do not exist when using UnoCSS? For example uno.css, virtual:uno.css, virtual:unocss-devtools

No, but specifiers starting with virtual: are ignored by default.

If uno.css is not referenced anywhere (not even it's executable/binary), then we could just return it from the resolveConfig function. It's a bit rare, but it happens. Here's an example: https://github.com/webpro/knip/blob/main/packages/knip/src/plugins/yorkie/index.ts#L26

@hyoban
Copy link
Contributor Author

hyoban commented May 18, 2024 •
edited
Loading

No, but specifiers starting with virtual: are ignored by default.

And we need to set virtual:uno.css in ignoreDependencies?

I added a test that will fail in packages/knip/fixtures/plugins/unocss/main.ts

@webpro
Copy link
Collaborator

webpro commented May 18, 2024

No, but specifiers starting with virtual: are ignored by default.

Sorry, I was wrong about this, they're not ignored. Here's what happened: #370

And we need to set virtual:uno.css in ignoreDependencies?

Yes, that's currently how it works indeed.

I added a test that will fail in packages/knip/fixtures/plugins/unocss/main.ts

There's two options (I don't mind which one):

  1. Accept there's an unlisted dependency (assert(issues.unlisted['main.ts']['virtual:uno.css']);)
  2. Add a Knip config to the fixtures folder with ignoreDependencies

@hyoban hyoban requested a review from webpro May 18, 2024 06:21
@webpro webpro merged commit 46ec674 into webpro-nl:main May 18, 2024
11 of 14 checks passed
@webpro
Copy link
Collaborator

webpro commented May 18, 2024

Thanks @hyoban!

@webpro
Copy link
Collaborator

webpro commented May 22, 2024

🚀 This pull request is included in v5.17.0-canary.0. See Release 5.17.0-canary.0 for release notes.

Using Knip in a commercial project? Please consider sponsoring me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webpro webpro Awaiting requested review from webpro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hyoban @webpro