search

react-webpack-generators / generator-react-webpack

Yeoman generator for ReactJS and Webpack
http://newtriks.com/2013/12/31/automating-react-with-yeoman-and-grunt/
MIT License
2.88k stars 355 forks source link

V4 -- Generate styleName attribute only when appropriate #255

Closed sthzg closed 8 years ago

sthzg commented 8 years ago

Currently the styleName attribute is added even when

sthzg commented 8 years ago

@weblogixx I've poked around a little and think that there are a few logical branches to support. + and - indicate the current support:

1) nostyle
    + no style import
    + no cssmodule annotation
    - no styleName attribute
    + no style file

2) styles, but cssmodules=false
    - style import/require, but no assignment to variable (=> eslint unused)
    - no cssmodule annotation
    - no styleName attribute
    - style file with naming scheme of <component>.<styletype> (e.g. foo.less)

3) styles, with cssmodules
    + style import with assignment to variable
    + cssmodule annotation
    + styleName attribute
    + style file with naming scheme of <component>.cssmodule.<styletype>

I think there needs to be a new flag added to the settings that can be used to branch between 2) and 3), for example:

settings.useCssModules = this.config.get('cssmodules') || false;

And then there is the issue of implementation.

It can be packed all into one template file (Base.js, Stateless.js). Pros: it remains in one file, Cons: ugly if-elses in the templates and no precise newline control. That means, to unify output it probably would have to be piped through some beautifier.

Another way would be a) working with multiple templates (BaseNoStyle.js, BaseStyles.js, BaseCssModules.js) or b) working with ejs-includes. Ejs-includes may be more DRY by the cost of readability, multiple templates aren't DRY and changes need to be reflected in a few files. However, I think this can be quite easily resolved with good tests, and base templates aren't subject to frequent changes. So from a maintainability POV my first thoughts are pro multiple base templates (= no ejs-includes, no extensive, newline-uncontrollable if-elses).

weblogixx commented 8 years ago

Hi @sthzg,

will have to think about it. Basically, if we want to use this, we should really create some simple default components with a maximum of one if statement (e.g. stateless, statefull, with cssmodules, without it...). Should be relatively easy to test, because the test suite can already handle different flags in its current incarnation.

sthzg commented 8 years ago

@weblogixx should I look into it and provide a PR or would you prefer keeping it in the queue for a bit until there is a final decision for it?

weblogixx commented 8 years ago

Hey @sthzg,

that would be really nice. There is too much in the pipeline for me alone currently :(. I think multiple base files would be better for this task. However, it could get complicated because we are also supporting the v3 version currently.

sthzg commented 8 years ago

@weblogixx ah, interesting. I was under the impression that the current /v3/** and /v4/** template structures would handle that separation. I will start implementing a solution using multiple base files and come back here when either there is sth. to show or to discuss if some blockers were too big to simply go on.

weblogixx commented 8 years ago

Hi @sthzg,

yes, indeed they are. However, version 3 only knows about regular and stateless components, not cssmodule ones. Thats the reason I was saying it :)

sthzg commented 8 years ago

@weblogixx I've just pushed an initial commit to coordinate the direction with you. The feature branch is here https://github.com/sthzg/generator-react-webpack/tree/feature/gh_issue_255

As noted in the commit message, first manual tests in v3 and v4 are working and, if the direction is okay, I'd work on updating the automated tests next (done), and then start detailed manual tests as well as probably iterating a bit over the code (done).

At first sight, it was funny to see that many different files, but to be honest I like working with them much more than with the ugly EJS conditions inside the templates (or a totally over-engineered AST processing for this purpose). I am currently of the opinion, that this is one case where verbosity actually has advantages over abstractions (those files will hardly ever change and if they do, it's easy to keep track of).

sthzg commented 8 years ago

Tests

V3

V4

Travis

Automated and manual tests all pass. @weblogixx if you are happy with the solution I could squash the branch and push a PR to see if Travis also likes it (seems to pass).

weblogixx commented 8 years ago

Pure genius 👍 . Looks really sweet and nicely implemented. Will have a deeper look at it today.

sthzg commented 8 years ago

I've now squashed the branch into one commit so that it is easier to review and if everything is fine ready to become a PR. Update: amended commit that updates assertion for app.cssmodule.css to app.css (due the template update to 2.0.1-4)

weblogixx commented 8 years ago

Just looked into it, hopefully awaiting your pr 👍

sthzg commented 8 years ago

Ready in https://github.com/newtriks/generator-react-webpack/pull/268