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

Remove unused class from jsx if the class is removed from the css module file #3

Closed
CobyPear opened this issue May 17, 2023 · 12 comments · Fixed by #4
Closed

Remove unused class from jsx if the class is removed from the css module file #3

CobyPear opened this issue May 17, 2023 · 12 comments · Fixed by #4
Labels
bug Something isn't working enhancement New feature or request

Comments

@CobyPear
Copy link

Hi, I have another request in order for this library to work a bit better for Gatsby related projects 😄.

Problem

Gatsby will throw errors if you import a style that does not exist in the css module file at build time.

Cannot read properties of undefined (reading 'e')
error Generating JavaScript bundles failed

For example, the jsx is left like so after the conversion: ${styles.navItemLink} text-blue-300 no-underline

But .navItemLink is removed from the css module file because all styles have been transformed to tailwindcss classes.
Gatsby will attempt to import .navItemLink from the file and throw an error because it is no longer present.

Proposed Solution

Remove the styles.className from the className={} if the class no longer exists in the module.

@shiyangzhaoa shiyangzhaoa added the enhancement New feature or request label May 19, 2023
@shiyangzhaoa
Copy link
Owner

shiyangzhaoa commented May 19, 2023

@CobyPear I wanted to solve this problem before but I was too lazy, and nobody asked me this question.

I tried to fix it, can you try the version 0.1.9-beta.1?

For some cases, I have done processing:

  1. className={style.className} => className='flex ...'
  2. className={clsx(style.className1, style.className2)} => className={clsx('flex ...', ...)}
  3. className={`${style.className1} ${style.className2}`} => className={`flex ...`}
  4. className={clsx({ [style.className]: true })} => className={clsx({ ['flex ...']: true })}

Thanks.

@shiyangzhaoa shiyangzhaoa added the bug Something isn't working label May 19, 2023
@CobyPear
Copy link
Author

Thank you for the quick response and implementation (again)!

This works really nicely.

There is one more case to solve for–when the *.module.css file is empty, it should be removed, and the import of that file in the jsx should also be removed. That way a project wouldn't have empty files or unused imports.

I understand if this is somewhat out of scope, but it would be a nice to have.

@shiyangzhaoa
Copy link
Owner

shiyangzhaoa commented May 20, 2023

Hi, you can try version 0.1.9-beta.2, because the import specifier is removed, the corresponding style member expressions also needs to be modified, for example:

/** index.module.css */
.test {
   display: flex;
}
import style from 'index.module.css';

const Component = () => (
   <div className={style.test}>
     <div className={style.invalid}></div>
   </div>
);

After removed:

const Component = () => (
   <div className='flex'>
     <div className={'invalid-class'}></div>
   </div>
);

Deleting import specifier is a dangerous operation, please make sure your code run successfully, thanks~

@CobyPear
Copy link
Author

CobyPear commented May 20, 2023

I tried 0.1.9-beta.2 and I got the same result as beta.1 where the *.css.module file is empty after the transform, but the import * from styles import specifier is still in the file, so I'm not seeing any change in the output. Is there any other config to tell it to remove the specifier? Maybe if it was opt + a warning when using it instead of default behavior it would be OK that it is a dangerous operation?

Also, please let me know if you accept sponsorships/donations through any platform as I am trying to get some $$ to you on behalf of my employer for all this work, thank you!

@shiyangzhaoa
Copy link
Owner

@CobyPear Can you try version 0.1.9-beta.4, I found a bug with an unhandled promise rejection in the code.

Thanks for your suggestion, if no one uses it, I don't bother to optimize it.

@CobyPear
Copy link
Author

CobyPear commented May 20, 2023

I am seeing the same result for 0.1.9-beta.4.

Thanks for your suggestion, if no one uses it, I don't bother to optimize it.

You're welcome, thanks for implementing it so quickly!

@shiyangzhaoa
Copy link
Owner

I need code samples that can reproduce the problem, such as https://codesandbox.io/

@CobyPear
Copy link
Author

CobyPear commented May 20, 2023

I am trying to get together a minimal reproduction for you, but it is a bit difficult since we are using your library via npx as a step in our CLI, and right before we run your library we run postcss on our css variables to transform those into something your library can read first (another feature that would be awesome if it was built in to your library 😄 ). I can outline the steps to setup our CLI locally but I worry it's a bit of work. I will try to get something less involved on Monday, but if you'd like I'll outline the steps I am using below. Please do not feel pressured to use these steps as I understand it is quite involved.

  1. Clone the following monorepo: git@github.com:pantheon-systems/decoupled-kit-js.git
  2. in the root of the monorepo, pnpm i you may need pnpm 8.1.0 You may see errors of things failing to build, you can ignore those. Make sure pnpm build:cli passes tho.
  3. On this line, change update the css-modules-to-tailwind command to the @0.1.9-beta.4 https://github.com/pantheon-systems/decoupled-kit-js/blob/canary/packages/create-pantheon-decoupled-kit/src/actions/convertCSSModules.ts#L45
  4. The, to call the CLI the easiest thing to do is to set up a watch script. I will copy one below that you can use. You may want to change the outDir to something else. the important part that will run the convert action is tailwindcss: true
  5. in the terminal, run pnpm watch:cli. You should see the CLI generate a new app then try to convert the CSS modules. The last linting step will fail due to styles being an unused var in the src/pages/404.jsx file. You can see the 404.module.css is empty

Here is the `watch.ts` file you will put at the root of `decoupled-kit-js/packages/create-pantheon-decoupled-kit`

import type { ParsedArgs } from 'minimist';
import path from 'path';
export const watchOptions: ParsedArgs = {
	_: ['gatsby-wp'],
	outDir: path.resolve(process.env.HOME as string, 'test-cli', 'css-mods-repro'),
	appName: 'css-modules-to-tw-repro',
	cmsEndpoint: 'https://any',
	noInstall: false,
	// force overwrite of the target directory.
	// WARNING: this option could overwrite uncommitted work.
	// Choose an empty outDir for best results with this options.
	force: true,
	// silent: true,
	// number of milliseconds to debounce file system watching
	tailwindcss: true,
	debounce: 5000,
};

@shiyangzhaoa
Copy link
Owner

Oh, I see. I read the file directly, the processed file has some newlines, file content converted to boolean is true, but it should be false... 😅

image

css-modules-to-tailwind@0.1.9-beta.5 will delete the import specifier, and css files are also removed.

@CobyPear
Copy link
Author

Nice, LGTM! Thanks again for the quick turnaround on this, it is very much appreciated!

@shiyangzhaoa
Copy link
Owner

Version 0.1.9, released.

@CobyPear
Copy link
Author

Thanks again, just ran another test and it works great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants