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 support for Babel plugins used in Vite/React config #904

Closed
wants to merge 1 commit into from

Conversation

mbacalan
Copy link

@mbacalan mbacalan commented Jan 8, 2025

Fixes #903

@mbacalan
Copy link
Author

mbacalan commented Jan 8, 2025

I tried to keep the changes minimal but had to copy over the resolveConfig fn from vitest plugin to add the additional plugin checks to it. I'm not sure if this is the way tbh. Looking forward to your feedback!

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 @mbacalan, much appreciated!

I'll enable CI and then it'll run in a couple of projects automatically as a "ecosystem smoke test".

@@ -48,7 +48,7 @@ const findConfigDependencies = (localConfig: ViteConfig, options: PluginOptions)
return [...[...environments, ...reporters, ...coverage].map(toDependency), ...setupFiles, ...globalSetup];
};

const getConfigs = async (localConfig: ViteConfigOrFn | VitestWorkspaceConfig) => {
export const getConfigs = async (localConfig: ViteConfigOrFn | VitestWorkspaceConfig) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we can import the babel function here and handle it from the vitest plugin?

}
}

return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't realize at first the babel object is sent to the react plugin, but if this works then... all good! :)

@@ -12,6 +18,52 @@ const isEnabled: IsPluginEnabled = ({ dependencies }) => hasDependency(dependenc

export const config = ['vite.config.{js,mjs,ts,cjs,mts,cts}'];

const hasReactBabelPlugins = (config: ViteConfig) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we can move it over to the vitest plugin, then this helper could go the helpers.ts

Copy link

pkg-pr-new bot commented Jan 8, 2025

Open in Stackblitz

npm i https://pkg.pr.new/knip@904

commit: 6279e26

@mbacalan
Copy link
Author

mbacalan commented Jan 8, 2025

Thanks for the feedback. Upon further inspection, it looks like these changes don't solve the issue.
I think I misunderstood the assertion in the test file. It actually means that we are getting 1 devDependencies error while the goal is to not receive any since we have @emotion/babel-plugin in package.json and vite.config.ts, right?

I'll get back to it soon and take into account the points you raised.

@webpro
Copy link
Collaborator

webpro commented Jan 8, 2025

Initially I saw babel: { plugins: ['@emotion/babel-plugin'] } as a prop of the Vite config and that would make things easy.

However, if the react() plugin doesn't just return that object argument (or anything useful) then we currently just don't have a solution for this. So, could you try and see if there's anything useful returned in the config object, just to be 100% sure?

It's basically the same problem as #856 where we also ideally would have an easy way to - instead of loading the configuration file and read as an object - use the AST and traverse it to find strings like "@emotion/babel-plugin". But that comes with a cost of performance impact and extra complexity.

At the very least this serves as an extra use case that probably involves reading and parsing config file ASTs.

@mbacalan
Copy link
Author

mbacalan commented Jan 8, 2025

I did a little deep dive and just came to the same conclusion :)

localConfig received by resolveConfig has plugins as [undefined]. I guess nothing much can be done about it without hardcoding some things and making a lot of assumptions.

Well, hopefully didn't take too much of your time here. Let me know if I should close this or feel free to do as you like. Nice codebase btw, learned a few things and it was fun to investigate through it!

@webpro
Copy link
Collaborator

webpro commented Jan 8, 2025

Thanks for digging through. And no worries about my time, mostly glad to hear you got something out of Knip!

@webpro webpro closed this Jan 8, 2025
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.

🐛Babel plugin passed to @vitejs/plugin-react reported as unused
2 participants