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 "meaninglessFileNames" option #304

Merged

Conversation

MeLlamoPablo
Copy link
Contributor

This PR adds a new option: meaninglessFileNames.

Consider the following use case:

  • A developer enables babel-plugin-styled-components with the defaults of displayName: true and fileName: true.

  • They organize their components with the following structure:

    src/
      components/
        MyComponent/
          index.js
          styles.js
    

    styles.js would be the file where they place their styled components.

  • For consistency, they always use Container for the root element of every component:

    // index.js
    import React from 'react';
    import { Container } from './styles.js';
    
    const MyComponent = () => <Container>Hello world!</Container>;

The class name generated for the Container element of MyComponent would be something like styles__Container-sc-000000-0. But it would be the same for the rest of the components of the application, because their styled components would be defined in styles.js too. So there would be no way to differentiate two Containers from different components.

The easy soultion would be to define the styled components in index.js, but for large components that can get messy very quick. Another solution would be to name each to prefix each styled component with the component name (i.e: MyComponentContainer), but that is very redundant and can make the code harder to read.

Actually, that is the exact use case of our company, and we found that many of our front-end developers ended up using the prefix solution. They preferred to compromise a bit on the readability of the code to allow for a better debugging experience.

This PR introduces the ideal solution: it allows to control when should the directory name be used for the component display name, and when it shouldn't, through the meaninglessFileNames option. By using the following options:

{
  displayName: true,
  fileName: true,
  meaninglessFileNames: ['index', 'styles']
},

our previous example would be labeled as MyComponent__Container-sc-000000-0, which is exactly what we need.

If this option is omitted, it defaults to ['index'], leaving the previous behaviour intact.

If this option is specified but either displayName or fileName are set to false, then it has no effect.

Some points to consider

  1. I didn't test this because I'm unsure on how to make this work with the test suite. It appears that the source file must be named code.js. How would I test that it works with different values? Any guidance would be appreciated.
  2. I'm unsure on whether or not meaninglessFileNames is the best naming. I considered ignoredFileNames, but that might be confusing as it might suggest that the files defined there would be ignored by the plugin. I also considered overruledFileNames, but I thought that meaninglessFileNames is what describes best what this option is about. Any thoughts?

Thanks for your time!

@MeLlamoPablo
Copy link
Contributor Author

Hello! Could I please get some feedback on this PR?

I understand if this feature doesn't align with the project's design goals, but I'd appreciate if anyone let me know whether this has any chance of getting merged. Of course, I'm willing to put more work into this.

Copy link
Collaborator

@quantizor quantizor left a comment

Choose a reason for hiding this comment

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

I like this a lot. Can you add some tests?

The "meaninglessFileNames" option provides the user with more precise
control over the component display name.

This option is to be used in conjunction with "displayName" and
"fileName". If either is set to false, this option will have no effect.

Prior to this commit, when both "displayName" and "fileName" were set,
the behaviour was to prepend the file name to the component display name
if it was named anything other than "index", and to prepend the
directory name otherwise.

The "meaninglessFileNames" option enables developers to control when
exactly the directory name should be used instead of the file name. If
the file name is considered to be "meaningless", that is, it doesn't
provide the developer with any useful information, then the directory
name will be used instead.

By default, the only "meaningless file name" is "index", which means
that the default behaviour is unmodified.
This test case checks whether the "meaninglessFileNames" option is
applied properly to the "code.js" file and its parent directory name is
being used instead of its file name.
@MeLlamoPablo
Copy link
Contributor Author

@probablyup rebased this branch on top of main and added some tests! Let me know if they look good :)

By the way, while digging deeper in the testing system I found that the case where the directory name isn't used when the file is named "index" is not being tested. I assume this is because babel-test doesn't allow us to change the code.js file name as they have it hardcoded.

Maybe it's worth sending them a PR to allow to change code.js to index.js for this specific case? Probably the author didn't think of an edge case where the filename would be relevant to the test itself.

@quantizor
Copy link
Collaborator

Maybe it's worth sending them a PR to allow to change code.js to index.js for this specific case? Probably the author didn't think of an edge case where the filename would be relevant to the test itself.

Love that idea!

@quantizor quantizor merged commit efb7a63 into styled-components:main Nov 26, 2021
@quantizor
Copy link
Collaborator

@MeLlamoPablo would you be interested in also contributing some documentation to https://github.com/styled-components/styled-components-website/blob/main/sections/tooling/babel-plugin.md?

@MeLlamoPablo
Copy link
Contributor Author

Absolutely! I'll send a PR soon

@MeLlamoPablo
Copy link
Contributor Author

Maybe it's worth sending them a PR to allow to change code.js to index.js for this specific case? Probably the author didn't think of an edge case where the filename would be relevant to the test itself.

Love that idea!

Hey! I know this is old, but better late than never! I submitted satya164/babel-test#9. Let's hear what the maintainer has to say.

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.

None yet

2 participants