Update Basic Generator & Linters

[Judahmeek]

This change is 

[Judahmeek]

Overlooked rspec. Fixing now.

[justin808]

HOWEVER.

Regarding the linter update...

We should make sure the generated code passes the linter.

We should NOT mix the lint changes for react on rails with the change to the generator.

---

Reviewed 15 of 15 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

lib/generators/react_on_rails/templates/no_redux/base/client/app/bundles/HelloWorld/startup/registration.jsx.tt, line 8 at r1 (raw file):

ReactOnRails.register({
  HelloWorld,
});

missing return

---

node_package/src/serverRenderReactComponent.js, line 29 at r1 (raw file):

        const redirectPath = redirectLocation.pathname + redirectLocation.search;
        console.log(`\
ROUTER REDIRECT: ${name} to dom node with id: ${domNodeId}, redirect to ${redirectPath}`,

I don't know if these will work. Node stuff is not updated to handle this.

---

Comments from Reviewable

[Judahmeek]

Review status: 14 of 18 files reviewed at latest revision, 2 unresolved discussions.

---

node_package/src/serverRenderReactComponent.js, line 29 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

> I don't know if these will work. Node stuff is not updated to handle this.

I assumed that Babel could handle this, but I'll change it.

---

Comments from Reviewable

[justin808]

Please confirm works locally, and travis passes. I might turn on codeship as well.

---

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-0.2%) to 82.818% when pulling c9af94a on Judahmeek:redux-gen into 03c5e96 on shakacode:master.

[coveralls]

Coverage increased (+0.02%) to 83.007% when pulling 8fccc92 on Judahmeek:redux-gen into 03c5e96 on shakacode:master.

[justin808]

Reviewed 1 of 20 files at r3.
Review status: 13 of 28 files reviewed at latest revision, 1 unresolved discussion.

---

package.json, line 58 at r1 (raw file):

    "flow": "flow check node_package",
    "lint": "npm run eslint && npm run flow",
    "lint:fix": "node_package/scripts/lint-fix",

why did you remove this?

However, I'd suggest:

1. Make lint only do eslint, so we can do npm run lint -- --fix
2. Move the flow part under the check task.

"check": "npm run lint && npm run flow && npm run test",

---

Comments from Reviewable

[justin808]

@Judahmeek I'd like to see us simplify this by taking out some of the options to put in the templates.

We should have the index.html.erb refer to "HelloWorldApp" for redux and "HelloWorld" for the simple no-redux.

Let's see what @robwise says.

---

Reviewed 8 of 20 files at r3, 11 of 12 files at r4.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

lib/generators/react_on_rails/react_no_redux_generator.rb, line 20 at r4 (raw file):

        }
        template(source + "/startup/registration.jsx" + ".tt", location + "/startup/registration.jsx", config)
        template(source + "/components/HelloWorld.jsx" + ".tt", location + "/components/HelloWorldApp.jsx", config)

I don't think we need the HelloWorldApp if there's no redux.

---

lib/generators/react_on_rails/react_with_redux_generator.rb, line 35 at r4 (raw file):

        %w(/startup/registration.jsx
           /components/HelloWorld.jsx).each do |file|
             template(base_path + destination + file + ".tt", destination + file, config)

normally, we prefer string interpolation rather than addition.

"#{base_path"}#{destination}"

---

lib/generators/react_on_rails/templates/base/base/client/app/bundles/HelloWorld/components/HelloWorld.jsx.tt, line 3 at r4 (raw file):

import React, { PropTypes } from 'react';

export default class <%= config[:class_name] %> extends React.Component {

I've no idea on why we're changing this to use the config[:class_name]

I'd rather see the source file called something like HelloWorldNoRedux.jsx.tt and have it copied to the right location.

---

Comments from Reviewable

[Judahmeek]

I see what you're suggesting. I should be able to make those changes pretty quickly.

---

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

package.json, line 58 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

why did you remove this?

However, I'd suggest:

1. Make lint only do eslint, so we can do npm run lint -- --fix
2. Move the flow part under the check task.

"check": "npm run lint && npm run flow && npm run test",

I removed that script and the lint-fix script file because the file encouraged the manual use of jscs, which has been rolled into eslint now making the script & file seem doubly irrelevant.

I'll add your suggested changes immediately.

---

lib/generators/react_on_rails/react_no_redux_generator.rb, line 20 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I don't think we need the HelloWorldApp if there's no redux.

Will fix.

---

lib/generators/react_on_rails/react_with_redux_generator.rb, line 35 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

normally, we prefer string interpolation rather than addition.

"#{base_path"}#{destination}"

Will fix. In my defense, most of the other code already here was string addition. I'll go ahead and fix all the other string addition code in the files I've been playing in.

---

lib/generators/react_on_rails/templates/base/base/client/app/bundles/HelloWorld/components/HelloWorld.jsx.tt, line 3 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I've no idea on why we're changing this to use the config[:class_name]

I'd rather see the source file called something like HelloWorldNoRedux.jsx.tt and have it copied to the right location.

The reason I made the class name conditional is because the current index.erb.html needs a HelloWorldApp component, but if HelloWorldContainer exists, then it looks for a HelloWorld component (and I figured that having a HelloWorldApp component in a HelloWorldApp container might cause issues, semantic and otherwise).

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.04%) to 83.028% when pulling 3503176 on Judahmeek:redux-gen into 03c5e96 on shakacode:master.

[justin808]

Awesome job @Judahmeek! I'll try this out and merge it!

---

Reviewed 1 of 21 files at r3, 2 of 13 files at r4, 11 of 11 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[justin808]

Thanks @Judahmeek!