Added css to css module script

[somya51p]

@jennydaman @PintoGideon I have created the script to convert CSS to CSS modules using regular expressions in js. Please review the PR and suggest changes if any.

[sherrif10]

@somya51p What exactly does this do. How can i test this generally.

[somya51p]

@somya51p What exactly does this do. How can i test this generally.

Hey, @sherrif10 It's associated with issue #167. This script would automatically convert CSS files to CSS modules.

[jennydaman]

Disclaimer: I haven't tried running this yet, I will try later.

[jennydaman]

What happens if multiple stylesheets are imported in the same JSX file? (Or how can you verify that this is not a concern?)

[somya51p]

What happens if multiple stylesheets are imported in the same JSX file? (Or how can you verify that this is not a concern?)

I analyzed all the files. As in the current codebase, a JSX file has a single CSS file, this script would easily work in our case so it should not be our concern at the very moment. Rest I will check later for the same.

[jennydaman]

Okay.

It would be helpful if you could provide a way to verify this is not a concern, i.e., explain how you did your "analysis."

I looked into this and here's a script I ran:

fd -e jsx -x bash -c '[ $(grep -F .css {} | grep -v node_modules | wc -l) -lt 2 ] || echo "WARNING: {} imports more than 1 local CSS file."' src

This script translates to: "for all JSX files under the src/ directory, count the number of lines which include the substring ".css" but do not include the substring "node_modules", and if this number is greater than or equal to 2, print a warning."

[somya51p]

Okay.

It would be helpful if you could provide a way to verify this is not a concern, i.e., explain how you did your "analysis."

I looked into this and here's a script I ran:

fd -e jsx -x bash -c '[ $(grep -F .css {} | grep -v node_modules | wc -l) -lt 2 ] || echo "WARNING: {} imports more than 1 local CSS file."' src

This script translates to: "for all JSX files under the src/ directory, count the number of lines which include the substring ".css" but do not include the substring "node_modules", and if this number is greater than or equal to 2, print a warning."

Ok, will take care of that, and also write unit tests. Thanks for the review.

[jennydaman]

do we need quotes around cls?

[somya51p]

do we need quotes around cls?

don't need that for normal className

[jennydaman]

During our meeting we talked about how we do need quotes here. I suggest you write examples and a unit test for this case.

[somya51p]

@jennydaman Please look into the changes made and tests created.

[somya51p]

Hey @jennydaman, to be precise here are the changes that I had made in the latest commits of this PR.

1. Created helper functions and added comments.
2. Added quotes for single className too.
3. Removed bug for multiple blacklisted classes.
4. Wrote tests for all the different test cases possible.

Please review it and let me know if I am missing out on something. Thankyou!

[jennydaman]

Hi @somya51p

I am still overwhelmed with a backlog of work but I see that you have worked very hard on this so I am providing some initial feedback now on this weekend. I will add more comments later.

[jennydaman]

This is a good use of recursion.

[jennydaman]

Notice that you have an "if/else if" here, which implies that your function is actually doing more than one thing, and it might be helpful to split it into a function for each thing it's doing.

Here, I would say that the function filesToChange is doing two things:

1. producing a list of all subpaths under a path
2. filtering a list of (sub)paths to produce a list of all files which end with .jsx

One option would be to use a library (or a different programming language): https://github.com/isaacs/node-glob

[jennydaman]

try to use the functions which are available to you, such as String.prototype.endsWith

[jennydaman]

When you make an argument "optional" it's actually a union-type between being "something" or null. Union types can be complicated because you must implement logic to handle both types. For these reasons, it is important that, whenever you have a union type or optional argument, you should provide some documentation indicating it as such.

[jennydaman]

Here's how I would've done it:

const fs = require('fs');
const path = require('path');

/**
 * @param {string} startingPath an existing path
 * @returns list of all subpaths under startingPath, including itself
 */
function findAllPaths(startingPath) {
  if (fs.statSync(startingPath).isDirectory()) {
    const subpaths = fs.readdirSync(startingPath).map((subpath) => {
      return findAllPaths(path.join(startingPath, subpath));
    });
    return Array.prototype.concat.apply([startingPath], subpaths);
  }
  return [startingPath];
}

// this helper function here has BOTH a self-explanatory name AND a simple implementation,
// so I am choosing to not add documentation
function filterByFileExtension(paths, suffix) {
  return paths.filter((p) => {
    return fs.statSync(p).isFile() && p.endsWith(suffix);
  });
}

// example

const exampleDirectory = 'ChRIS_store_ui/src';

filterByFileExtension(findAllPaths(exampleDirectory), '.jsx').forEach(p => {
  console.log(p);
});

I've identified the individual units of functionality of this task and created a helper for each one. This way, each function is individually testable, so if either one is doing the wrong thing, it's much easier to identify where the bug is in my code, and also the fix for my code will be much smaller. It can also be easier for others to read, understand, and review my code, because the names of my functions are descriptive and there is docstring where necessary.

[somya51p]

Hi @somya51p

I am still overwhelmed with a backlog of work but I see that you have worked very hard on this so I am providing some initial feedback now on this weekend. I will add more comments later.

Ohk, Thanks a lot for your time and review :)
I will study your feedback and implement it in the next commit.