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

Update CONTRIBUTING.md and related files #737

Merged
merged 1 commit into from
Mar 16, 2017

Conversation

wanyamaman
Copy link
Contributor

@wanyamaman wanyamaman commented Feb 24, 2017

  • Explicitly lists the locations to run commands when setting up the gem for local development.
  • Fixes an issue where yarn install-react-on-rails command used yarn instead of yarn add and failed to exectute.

Closes #736


This change is Reviewable

@coveralls
Copy link

coveralls commented Feb 24, 2017

Coverage Status

Coverage remained the same at 99.329% when pulling 81d2fd6 on wanyamaman:update-contributing into 9a198bf on shakacode:master.

@justin808
Copy link
Member

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


CONTRIBUTING.md, line 74 at r1 (raw file):

* /spec/react_on_rails/dummy-for-generators

# Configuring your test app to use your local fork

A test app could be a brand new rails app configured with the React on Rails gem.


CONTRIBUTING.md, line 100 at r1 (raw file):

When you use a relative path, be sure to run the above `yarn` command whenever you change the node package for react-on-rails.

Why is all of this being removed?

Unless there is a specific reason for removing one of these steps, please keep them in.

OK -- I see, you believe this should go below?


CONTRIBUTING.md, line 152 at r1 (raw file):

```sh
# react_on_rails/spec/dummy/client

maybe just make it clear that this is directory to cd to?


CONTRIBUTING.md, line 164 at r1 (raw file):

### Starting the Dummy App
To run the dummy app, it's **CRITICAL** to not just run `rails s`. You have to run `foreman start`. If you don't do this, then `webpack` will not generate a new bundle, and you will be seriously confused when you change JavaScript and the app does not change. If you change the webpack configs, then you need to restart foreman. If you change the JS code for react-on-rails, you need to run `yarn run build`. Since the react-on-rails package should be sym linked, you don't have to `yarn react-on-rails` after every change.

The spec/dummy is a special "test app" that lives inside of the gem and is used for some tests. We also have direct gem tests and test of the generator.


CONTRIBUTING.md, line 170 at r1 (raw file):

```sh
cd spec/dummy

yes, you need to cd here to run these tests


CONTRIBUTING.md, line 177 at r1 (raw file):

```sh
cd spec/dummy/client

yes, you need to be here


CONTRIBUTING.md, line 220 at r1 (raw file):

Run `docker-compose build tests` to build the tests container. Run `docker-compose run tests` to start all RSpec tests.

# Configuring your test app to use your local fork

I guess we can make it clear what's are the different types of test apps.


spec/dummy/client/package.json, line 77 at r1 (raw file):

    "build:server": "webpack --config webpack.server.rails.build.config.js",
    "hot-assets": "babel-node server-rails-hot.js",
    "install-react-on-rails": "yarn add 'file:../../..'"

this looks good if you tried it!

and please be sure that when you change the JS part of the npm package, that just running this command works...

I'm not 100% sure that this runs the build process on the package.


Comments from Reviewable

@wanyamaman
Copy link
Contributor Author

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


CONTRIBUTING.md, line 100 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why is all of this being removed?

Unless there is a specific reason for removing one of these steps, please keep them in.

OK -- I see, you believe this should go below?

I'm not entirely comfortable with it being all the way at the bottom too. But it did confuse me by the time I reached the section on Development Setup for Gem and Node Package Contributors. You'd have been introduced to the test app outside the gem, the gem root itself as well as the dummy app. That's three different places you can run yarn commands.

Alternatively it can stay in its original place but we must be clear on what is a test app vs a dummy app vs example app and where commands are to be run.


CONTRIBUTING.md, line 164 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

The spec/dummy is a special "test app" that lives inside of the gem and is used for some tests. We also have direct gem tests and test of the generator.

Thanks for the confirmation. I managed to figure it out eventually. However, it does get confusing if we sometimes call it the dummy app and at other places the test app since we have test apps that live outside the gem as well. Do you think we should be able to distinguish between the two?


CONTRIBUTING.md, line 220 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I guess we can make it clear what's are the different types of test apps.

Yes. This is where most of the confusion stems from.


spec/dummy/client/package.json, line 77 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

this looks good if you tried it!

and please be sure that when you change the JS part of the npm package, that just running this command works...

I'm not 100% sure that this runs the build process on the package.

Gotcha. I'll confirm the changes. Just to be sure, we want this to be the equivalent of running yarn and yarn run build at the root directory of the gem. Is that correct?


Comments from Reviewable

@justin808
Copy link
Member

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 100 at r1 (raw file):

Previously, wanyamaman (William Wanyama) wrote…

I'm not entirely comfortable with it being all the way at the bottom too. But it did confuse me by the time I reached the section on Development Setup for Gem and Node Package Contributors. You'd have been introduced to the test app outside the gem, the gem root itself as well as the dummy app. That's three different places you can run yarn commands.

Alternatively it can stay in its original place but we must be clear on what is a test app vs a dummy app vs example app and where commands are to be run.

I think up at the top is good as this setup is used for /spec/dummy.

Let's make it very clear that the /spec/dummy is one test case for react on rails, and an example of the various techniques!


CONTRIBUTING.md, line 164 at r1 (raw file):

Previously, wanyamaman (William Wanyama) wrote…

Thanks for the confirmation. I managed to figure it out eventually. However, it does get confusing if we sometimes call it the dummy app and at other places the test app since we have test apps that live outside the gem as well. Do you think we should be able to distinguish between the two?

we can just call it the spec/dummy app


CONTRIBUTING.md, line 170 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

yes, you need to cd here to run these tests

Please make it clear you have to be in these directories.


spec/dummy/client/package.json, line 77 at r1 (raw file):

Previously, wanyamaman (William Wanyama) wrote…

Gotcha. I'll confirm the changes. Just to be sure, we want this to be the equivalent of running yarn and yarn run build at the root directory of the gem. Is that correct?

test it out...try it.


Comments from Reviewable

@justin808
Copy link
Member

a few comments...


Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


Comments from Reviewable

@coveralls
Copy link

coveralls commented Feb 26, 2017

Coverage Status

Coverage remained the same at 99.329% when pulling e1e0e29 on wanyamaman:update-contributing into 9a198bf on shakacode:master.

@justin808
Copy link
Member

@wanyamaman You've got some odd things going on with your git commits. I want to see a PR with ONE commit with your changes.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.329% when pulling 80f4f14 on wanyamaman:update-contributing into 8e47c79 on shakacode:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.329% when pulling 80f4f14 on wanyamaman:update-contributing into 8e47c79 on shakacode:master.

@justin808
Copy link
Member

This is looking FABULOUS. Let's just clarify so that the noob knows that there's 2 parts for the local development reference: 1) ruby part, 2) node part.


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 12 unresolved discussions.


CONTRIBUTING.md, line 82 at r2 (raw file):

    └── spec
        └── dummy

nifty graphic!


CONTRIBUTING.md, line 85 at r2 (raw file):

to test the gem

to test the ruby parts of the gem

and let's be clear on needing to run either the ruby part or the the NPM part depending on if you want to test the ruby or npm parts or BOTH.


CONTRIBUTING.md, line 105 at r2 (raw file):

```sh
cd test/client
yarn add "react-on-rails@../../react_on_rails"

did you try this?


CONTRIBUTING.md, line 108 at r2 (raw file):

When you use a relative path, be sure to run the above yarn command whenever you change the node package for react-on-rails.

and not just running yarn install as that might not do anything unless the underlying package.json version number changes.


spec/dummy/client/package.json, line 77 at r2 (raw file):

    "build:server": "webpack --config webpack.server.rails.build.config.js",
    "hot-assets": "babel-node server-rails-hot.js",
    "install-react-on-rails": "CLIENT_PATH=$(pwd) && cd ../../../ && yarn && cd $CLIENT_PATH && yarn add 'file:../../..'"

Is the use of the CLIENT_PATH variable required? Nifty if this what it took.

we should probably assume that the script yarn run install-react-on-rails is run from the same directory...

anyway if this is required, super....

if not, then let's remove.


Comments from Reviewable

@wanyamaman
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 74 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

A test app could be a brand new rails app configured with the React on Rails gem.

Done.


CONTRIBUTING.md, line 100 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think up at the top is good as this setup is used for /spec/dummy.

Let's make it very clear that the /spec/dummy is one test case for react on rails, and an example of the various techniques!

Done.


CONTRIBUTING.md, line 152 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

maybe just make it clear that this is directory to cd to?

Done.


CONTRIBUTING.md, line 170 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please make it clear you have to be in these directories.

Done.


CONTRIBUTING.md, line 177 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

yes, you need to be here

Done.


CONTRIBUTING.md, line 85 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

to test the gem

to test the ruby parts of the gem

and let's be clear on needing to run either the ruby part or the the NPM part depending on if you want to test the ruby or npm parts or BOTH.

I'm not sure what the overall output of this section is meant to be. Please clarify


CONTRIBUTING.md, line 93 at r2 (raw file):

Note that you will need to bundle install after making this change, but also that **you will need to restart your Rails application if you make any changes to the gem**.

## NPM for react-on-rails

This section needs more information. Is this the part when the user wants to test the npm part of the gem. Do we assume they've already installed gem or is this a continuous sequence from the Ruby gem section above?


CONTRIBUTING.md, line 105 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

did you try this?

Yes, this adds the following line to the client/package.json dependencies ->
"react-on-rails": "../../react_on_rails",


CONTRIBUTING.md, line 108 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

and not just running yarn install as that might not do anything unless the underlying package.json version number changes.

doesn't the command yarn call yarn install if you don't pass it any commands?


spec/dummy/client/package.json, line 77 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Is the use of the CLIENT_PATH variable required? Nifty if this what it took.

we should probably assume that the script yarn run install-react-on-rails is run from the same directory...

anyway if this is required, super....

if not, then let's remove.

It depends. If we will have other dummy apps besides the spec dummy then this is a universal script that will work with them all. Otherwise we can hard code the path down from the gem root to the spec/dummy -> which would be:
cd ../../../ && yarn && cd spec/dummy/client && yarn add 'file:../../../'


Comments from Reviewable

@coveralls
Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage remained the same at 99.329% when pulling 41311b6 on wanyamaman:update-contributing into 8e47c79 on shakacode:master.

@justin808
Copy link
Member

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.


CONTRIBUTING.md, line 85 at r2 (raw file):

Previously, wanyamaman (William Wanyama) wrote…

I'm not sure what the overall output of this section is meant to be. Please clarify

Hi @wanyamaman, the idea is that you will change either the JS part or the Ruby part or both. Depending on which you change, you need to link to the relevant part. One other detail is that the Ruby part just requires restarting the server. The JavaScript part requires re-running the install script and refreshing the browser page.


CONTRIBUTING.md, line 93 at r2 (raw file):
Hi @wanyamaman! Yes!

Is this the part when the user wants to test the npm part of the gem


CONTRIBUTING.md, line 108 at r2 (raw file):

Previously, wanyamaman (William Wanyama) wrote…

doesn't the command yarn call yarn install if you don't pass it any commands?

Yes, yarn does do yarn install by default. Older versions of npm DID NOT update the JS files under node_modules for a locally referenced directory each time. This may have changed with yarn. The way to check is to make a JS file change in this directory: https://github.com/shakacode/react_on_rails/tree/master/node_package/src and to then run the yarn command with no parameters and to see if the file under /client/node_modules in your test app changed.


spec/dummy/client/package.json, line 77 at r2 (raw file):

Previously, wanyamaman (William Wanyama) wrote…

It depends. If we will have other dummy apps besides the spec dummy then this is a universal script that will work with them all. Otherwise we can hard code the path down from the gem root to the spec/dummy -> which would be:
cd ../../../ && yarn && cd spec/dummy/client && yarn add 'file:../../../'

I think we should assume this command is specific to this directory. I'm guessing that the cd to the top level directory is needed to build the node package?

I'm surprised we cannot do something like just running the command that you mention above:

yarn add "react-on-rails@../../../react_on_rails"

(number of directories up might not be right)


Comments from Reviewable

@wanyamaman
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 164 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

we can just call it the spec/dummy app

Done.


CONTRIBUTING.md, line 108 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Yes, yarn does do yarn install by default. Older versions of npm DID NOT update the JS files under node_modules for a locally referenced directory each time. This may have changed with yarn. The way to check is to make a JS file change in this directory: https://github.com/shakacode/react_on_rails/tree/master/node_package/src and to then run the yarn command with no parameters and to see if the file under /client/node_modules in your test app changed.

Hmm I'm not getting the file in client/modules to change. Thoughts?


spec/dummy/client/package.json, line 77 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

test it out...try it.

Done.


spec/dummy/client/package.json, line 77 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think we should assume this command is specific to this directory. I'm guessing that the cd to the top level directory is needed to build the node package?

I'm surprised we cannot do something like just running the command that you mention above:

yarn add "react-on-rails@../../../react_on_rails"

(number of directories up might not be right)

Unfortunately, using yarn add alone isn't triggering the build process for the gem but it does install it in dummy apps dependency list.

I'll change the command to be directory specific.

The number of directories is fine but lets change it to be more explicit when reading


Comments from Reviewable

@justin808
Copy link
Member

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 108 at r2 (raw file):

Previously, wanyamaman (William Wanyama) wrote…

Hmm I'm not getting the file in client/modules to change. Thoughts?

this is what needs to be clear.

I usually use a command like:

https://github.com/shakacode/react_on_rails/blob/master/spec/dummy/package.json#L14

    "install-react-on-rails": "cd client && yarn run install-react-on-rails",

Comments from Reviewable

@wanyamaman
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.


CONTRIBUTING.md, line 82 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

nifty graphic!

thanks


CONTRIBUTING.md, line 85 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Hi @wanyamaman, the idea is that you will change either the JS part or the Ruby part or both. Depending on which you change, you need to link to the relevant part. One other detail is that the Ruby part just requires restarting the server. The JavaScript part requires re-running the install script and refreshing the browser page.

I've added clarification.


CONTRIBUTING.md, line 108 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

this is what needs to be clear.

I usually use a command like:

https://github.com/shakacode/react_on_rails/blob/master/spec/dummy/package.json#L14

    "install-react-on-rails": "cd client && yarn run install-react-on-rails",

Reverted back to npm.


Comments from Reviewable

@coveralls
Copy link

coveralls commented Mar 2, 2017

Coverage Status

Coverage remained the same at 99.33% when pulling 82754ce on wanyamaman:update-contributing into c294768 on shakacode:master.

@justin808
Copy link
Member

some comments

getting closer!


Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.


CONTRIBUTING.md, line 85 at r4 (raw file):

Test Ruby Gem

Testing the Ruby Gem


CONTRIBUTING.md, line 94 at r4 (raw file):

Note that you will need to bundle install after making this change, but also that **you will need to restart your Rails application if you make any changes to the gem**.

## Test NPM for react-on-rails

Testing the Node package for react-on-rails


CONTRIBUTING.md, line 95 at r4 (raw file):

You can also test the npm parts of the gem with an external application.

In addition to testing the Ruby parts out, you can test the node package parts of the gem with an external application.


CONTRIBUTING.md, line 107 at r4 (raw file):

cd test/client
yarn remove react-on-rails

was this necessary? did you try just running the npm command?


CONTRIBUTING.md, line 114 at r4 (raw file):

npm i --save 'react-on-rails@../path-to-react-on-rails'

Note: You must use npm here till yarn stops preferring cached packages over local. see issue

excellent! but put in issue number!

_Note: You must use npm here till yarn stops preferring cached packages over local. see [yarn issue 2649](https://github.com/yarnpkg/yarn/issues/2649)_

spec/dummy/client/package.json, line 36 at r4 (raw file):

    "react": "^15.4.1",
    "react-dom": "^15.4.1",
    "react-on-rails": "file:../../../../react_on_rails",

Is this necessary to put in react_on_rails? what if the project is cloned under a different name?

less dots and no react_on_rails maybe!


spec/dummy/client/package.json, line 77 at r4 (raw file):

    "build:server": "webpack --config webpack.server.rails.build.config.js",
    "hot-assets": "babel-node server-rails-hot.js",
    "install-react-on-rails": "npm install --save 'react-on-rails@file:../../../../react_on_rails'"

If you really need to delete, then you can add this to the script here.

Is this necessary to put in react_on_rails? what if the project is cloned under a different name?

less dots and no react_on_rails maybe!


Comments from Reviewable

@justin808
Copy link
Member

@wanyamaman Please let me know when you've addressed my comments!

@justin808
Copy link
Member

@wanyamaman Is this ready for merging?

@wanyamaman
Copy link
Contributor Author

@justin808 updated


Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions.


CONTRIBUTING.md, line 85 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Testing the Ruby Gem

Done.


CONTRIBUTING.md, line 94 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Testing the Node package for react-on-rails

Done.


CONTRIBUTING.md, line 95 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

You can also test the npm parts of the gem with an external application.

In addition to testing the Ruby parts out, you can test the node package parts of the gem with an external application.

Done.


CONTRIBUTING.md, line 107 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

was this necessary? did you try just running the npm command?

It's not necessary, this was used to clean the yarn.lock file for those who've already ran the install using yarn instead of npm.


CONTRIBUTING.md, line 114 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

excellent! but put in issue number!

_Note: You must use npm here till yarn stops preferring cached packages over local. see [yarn issue 2649](https://github.com/yarnpkg/yarn/issues/2649)_

Done.


spec/dummy/client/package.json, line 36 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Is this necessary to put in react_on_rails? what if the project is cloned under a different name?

less dots and no react_on_rails maybe!

Done.


spec/dummy/client/package.json, line 77 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

If you really need to delete, then you can add this to the script here.

Is this necessary to put in react_on_rails? what if the project is cloned under a different name?

less dots and no react_on_rails maybe!

Done.


Comments from Reviewable

@coveralls
Copy link

coveralls commented Mar 7, 2017

Coverage Status

Coverage remained the same at 97.898% when pulling 50d10b6 on wanyamaman:update-contributing into 5872522 on shakacode:master.

@justin808
Copy link
Member

One question...couple minor comments!

Close!


Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


CONTRIBUTING.md, line 113 at r5 (raw file):

#### Example: Testing NPM changes with the dummy app
1. Add `console.log('Hello!')` to top of the `clientStartup` function in `react_on_rails/node_package/src/clientStartup.js`. This is our desired change.

file, not function


CONTRIBUTING.md, line 115 at r5 (raw file):

1. Add `console.log('Hello!')` to top of the `clientStartup` function in `react_on_rails/node_package/src/clientStartup.js`. This is our desired change.
2. Run `yarn` and `yarn run build` from the gem root directory: `react_on_rails/`. This compiles our changes.
3. Run the install script `npm run install-react-on-rails` in `react_on_rails/spec/dummy` to copy over our changes to the dummy app. Alternatively, you can run `npm i --save 'file:../../../'` in `react_on_rails/spec/dummy/client`. Our NPM changes are now available in the dummy app.

Are we sure that the npm i does not run the build command? I'm pretty sure it used to. We should check if we can remove step 2.


CONTRIBUTING.md, line 116 at r5 (raw file):

2. Run `yarn` and `yarn run build` from the gem root directory: `react_on_rails/`. This compiles our changes.
3. Run the install script `npm run install-react-on-rails` in `react_on_rails/spec/dummy` to copy over our changes to the dummy app. Alternatively, you can run `npm i --save 'file:../../../'` in `react_on_rails/spec/dummy/client`. Our NPM changes are now available in the dummy app.
4. Refresh the browser if the server is already running or start the server using `foreman start` from `react_on_rails/spec/dummy` and navigate to `http://localhost:3000/`. You will now see the `[SERVER] Hello!` message printed in the browser's console.

that's is only if you have server rendering turned on

and you modified the clientStartup file


Comments from Reviewable

@wanyamaman
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 115 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Are we sure that the npm i does not run the build command? I'm pretty sure it used to. We should check if we can remove step 2.

It does but yarn doesn't. Should we cater for yarn setups or remove that step and the yarn build instruction above until yarn fixes their caching issue?


CONTRIBUTING.md, line 116 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

that's is only if you have server rendering turned on

and you modified the clientStartup file

Should that be added as a footnote? The assumption here is that you haven't modified spec/dummy so server rendering is on and you've modified the clientStartup file as described.

The whole point of this section is to give the reader a general reproducible example highlighting the flow of execution when making a change to the npm package. Unless you'd like to cover all possible configurations they should be able to deduce the changes in their specific configuration.


Comments from Reviewable

@justin808
Copy link
Member

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 115 at r5 (raw file):

Previously, wanyamaman (William Wanyama) wrote…

It does but yarn doesn't. Should we cater for yarn setups or remove that step and the yarn build instruction above until yarn fixes their caching issue?

I'd mention that, so we know that when yarn fixes that, we'll remove that.


CONTRIBUTING.md, line 116 at r5 (raw file):

Previously, wanyamaman (William Wanyama) wrote…

Should that be added as a footnote? The assumption here is that you haven't modified spec/dummy so server rendering is on and you've modified the clientStartup file as described.

The whole point of this section is to give the reader a general reproducible example highlighting the flow of execution when making a change to the npm package. Unless you'd like to cover all possible configurations they should be able to deduce the changes in their specific configuration.

I'd keep this all client side, and if the client side works, we'll know the server side works.

so your example with [SERVER] is not right.


Comments from Reviewable

@justin808
Copy link
Member

@wanyamaman Would you like to address my last few comments and we'll merge?

@wanyamaman
Copy link
Contributor Author

Addressed


Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.


CONTRIBUTING.md, line 113 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

file, not function

updated


CONTRIBUTING.md, line 115 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I'd mention that, so we know that when yarn fixes that, we'll remove that.

I've removed it and left a footnote on how to do it with yarn once the issue is fixed.


CONTRIBUTING.md, line 116 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I'd keep this all client side, and if the client side works, we'll know the server side works.

so your example with [SERVER] is not right.

Moved to client side.


Comments from Reviewable

@coveralls
Copy link

coveralls commented Mar 14, 2017

Coverage Status

Coverage remained the same at 97.898% when pulling 444f048 on wanyamaman:update-contributing into f42e8f4 on shakacode:master.

@justin808
Copy link
Member

:lgtm:


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


CONTRIBUTING.md, line 113 at r6 (raw file):

  1. Add console.log('Hello!') here in react_on_rails/node_package/src/clientStartup.js. This is our desired change.
1. Add `console.log('Hello!')` [here](https://github.com/shakacode/react_on_rails/blob/master/node_package/src/clientStartup.js#L181) in `react_on_rails/node_package/src/clientStartup.js` to confirm we're getting an update to the node package.

I can do this update.


CONTRIBUTING.md, line 115 at r6 (raw file):

1. Add `console.log('Hello!')` [here](https://github.com/shakacode/react_on_rails/blob/master/node_package/src/clientStartup.js#L181) in `react_on_rails/node_package/src/clientStartup.js`. This is our desired change.
2. Run the install script `npm run install-react-on-rails` in `react_on_rails/spec/dummy` to copy over our changes to the dummy app. Alternatively, you can run `npm i --save 'file:../../../'` in `react_on_rails/spec/dummy/client`. Our NPM changes are now available in the dummy app.
3. Refresh the browser if the server is already running or start the server using `foreman start` from `react_on_rails/spec/dummy` and navigate to `http://localhost:5000/`. You will now see the `Hello!` message printed in the browser's console.

using foreman start -f Profile.dev


Comments from Reviewable

@justin808 justin808 merged commit 9efc551 into shakacode:master Mar 16, 2017
@wanyamaman wanyamaman deleted the update-contributing branch March 16, 2017 08:37
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.

3 participants