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

WIP: Remove babel-plugin-react-css-modules a.k.a. styleName vs className #9

Conversation

neonnoon
Copy link
Contributor

@neonnoon neonnoon commented Apr 7, 2021

Issues are not enabled on this repo, therefore I submit this proposal as a (partial) PR.
The goal here is to get rid of babel-plugin-react-css-modules, because:

In order to stay future proof and reduce dependencies on abandoned projects I suggest that we move from styleName to className. In addition to that IDE support is able to detect proper class names (as they're variable names as opposed to string literals).

Please lemme know what you think of this proposal and I'll have a look at it.

@neonnoon neonnoon self-assigned this Apr 7, 2021
@neonnoon neonnoon changed the title Remove babel-plugin-react-css-modules WIP: Remove babel-plugin-react-css-modules Apr 12, 2021
@sonarcloud
Copy link

sonarcloud bot commented Apr 12, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@neonnoon neonnoon changed the title WIP: Remove babel-plugin-react-css-modules WIP: Remove babel-plugin-react-css-modules a.k.a. styleName vs className Apr 12, 2021
@sosostris
Copy link
Contributor

Happy snowman-burning Day @neonnoon ! ⛄

We were discussing today about the upcoming clsx PR. Our idea is,

  1. For pyrene, we do everything in one big PR, including
  • introducing clsx to pyrene
  • convert all pyrene components to use clsx
  1. Then we need two separate PRs to handle:
  • tuktuktwo
  • pyrene-graphs

@Sakai-san please correct me if I'm wrong :)

I have two questions:

  1. For the pyrene part, would you prefer that we work on top of this PR (e.g. making new commits to this PR), or it's better that we start a separate PR?
  2. pyrene, pyrene-graphs and tuktuktwo will all have their dependency on clsx, could peer dependency be somehow employed to alleviate the issue that at some point there will be 3 different versions of clsx?

@Sakai-san Sakai-san requested a review from sanmiawi April 19, 2021 19:56
@Sakai-san
Copy link
Contributor

@Sakai-san please correct me if I'm wrong :)

I have two questions:

  1. For the pyrene part, would you prefer that we work on top of this PR (e.g. making new commits to this PR), or it's better that we start a separate PR?
  2. pyrene, pyrene-graphs and tuktuktwo will all have their dependency on clsx, could peer dependency be somehow employed to alleviate the issue that at some point there will be 3 different versions of clsx?

@sosostris, @sanmiawi

  1. I would create a brand new PR branched off that present PR for all the remaining components.
  2. I would not treat it as peerDependency since the main project does not have any package.json.

@neonnoon
Copy link
Contributor Author

  • I would create a brand new PR branched off that present PR for all the remaining components.

Whatever works best for you. Generally, I think we have a bit of a mess with PRs. If you wanna branch off this branch for further changes, I think we can just kill this PR (the branch will remain and can be used as a base).

  • I would not treat it as peerDependency since the main project does not have any package.json.

What do you mean my main project?

I would add it as a dependency, not peerDependency. A peer dependency makes sense when the dependent project, (e.g. an mc app, or an app I am writing) must use a certain dependency. That makes sense for react because using pyrene without react doesn't make much sense. For clsx, however, pyrene cannot and should not make any assumptions of how parent projects will build class names. Therefore this should be a dependency.

Also, please note that if we assume clsx to be provided by the parent project, it should also be externalized in webpack:

externals: ['react', 'react-dom', 'moment', 'moment-timezone'],

But, again, I really don't see the point why an application using pyrene should have to provide clsx.

@justmejulian
Copy link
Contributor

@Sakai-san and @sanmiawi is this still needed or can it be declined?

@neonnoon
Copy link
Contributor Author

@Sakai-san and @sanmiawi is this still needed or can it be declined?

If all the styleNames have been migrated, and #91 is merged, and babel-plugin-react-css-modules is removed as a dependency, I think we can kill this one.

@neonnoon
Copy link
Contributor Author

Done with #91 (and related changes).

@neonnoon neonnoon closed this May 14, 2021
@neonnoon neonnoon deleted the feature/remove-react-css-modules-and-style-names branch May 14, 2021 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants