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.

Sign up for GitHub

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

Jump to bottom

Add React Hot Loader 3 #392

Merged
merged 1 commit into from
Mar 30, 2017
Merged

Add React Hot Loader 3 #392

merged 1 commit into from
Mar 30, 2017

Conversation

oshalygin
Copy link
Contributor

@oshalygin oshalygin commented Mar 14, 2017 •
edited
Loading

  • This commit wires up react-hot-loader 3 to
    be used in the application. There are numerous
    benefits to the latest release, all of which
    can be seen at https://github.com/gaearon/react-hot-loader
  • Note that the specific implementation around
    wrapping in a Root component is part of how
    react-hot-loader 3 needs to be configured.
  • Note that the package is brought in as a
    dependency, not a dev dependency because of
    how it is switched at runtime or not.
  • More information on the migration can be
    viewed at:
    https://github.com/gaearon/react-hot-loader/tree/master/docs

Related: #216

@oshalygin oshalygin changed the title Add react hot loader 3 Add React Hot Loader 3 Mar 14, 2017
@coveralls
Copy link

coveralls commented Mar 14, 2017 •
edited
Loading

Coverage Status

Changes Unknown when pulling 3865ef7 on oshalygin:feature/add-hmr-3 into ** on coryhouse:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9203ee3 on oshalygin:feature/add-hmr-3 into ** on coryhouse:master**.

1 similar comment
@coveralls
Copy link

coveralls commented Mar 14, 2017 •
edited
Loading

Coverage Status

Changes Unknown when pulling 9203ee3 on oshalygin:feature/add-hmr-3 into ** on coryhouse:master**.

@coryhouse
Copy link
Owner

Excellent! Thanks for the PR! This looks great to me but I'm going to let it sit for a couple days so hopefully others can chime in. Thanks again!

@oshalygin
Copy link
Contributor Author

Sounds good thanks @coryhouse !

@kwelch
Copy link
Collaborator

kwelch commented Mar 20, 2017

I put in my comment on the version number, other than that all appears to be accurate.

@oshalygin
Copy link
Contributor Author

@kwelch watcha mean by version number? I didn't see your comment :)

@kwelch
Copy link
Collaborator

kwelch commented Mar 20, 2017 •
edited
Loading

In the package.json instead of having a specific version number mentioned it just says "next".

It is an oddity with yarn, I put in a ticket yarnpkg/yarn#1306, but it appears to have resurfaced. Additionally, yarn does not use the .npmrc so it does not save exact and as the point that this was last discussed in #363, they had not set on a solution.

@oshalygin
Copy link
Contributor Author

I'll update it, thanks for the tip!

@oshalygin
Add react hot loader 3
b1ab8ae 
- This commit wires up react-hot-loader 3 to
  be used in the application.  There are numerous
  benefits to the latest release, all of which
  can be seen at https://github.com/gaearon/react-hot-loader
- Note that the specific implementation around
  wrapping in a Root component is part of how
  react-hot-loader 3 needs to be configured.
- Note that the package is brought in as a
  dependency, not a dev dependency because of
  how it is switched at runtime or not.
- More information on the migration can be
  viewed at:
https://github.com/gaearon/react-hot-loader/tree/master/docs\#migration-to-30

Related: coryhouse#216
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b1ab8ae on oshalygin:feature/add-hmr-3 into ** on coryhouse:master**.

kwelch
kwelch approved these changes Mar 21, 2017
View reviewed changes
Copy link
Collaborator

@kwelch kwelch left a comment

Choose a reason for hiding this comment

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

Looks good all versions are properly using exact versions.

package.json Outdated
@@ -77,6 +77,7 @@
"postcss-loader": "1.2.1",
"prompt": "1.0.0",
"react-addons-test-utils": "15.4.1",
"react-hot-loader": "next",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use exact version numbers. Thanks. This is set in our .npmrc, which is not currently respected by yarn.

@oshalygin
Copy link
Contributor Author

thanks @kwelch

@kwelch
Copy link
Collaborator

kwelch commented Mar 22, 2017

Thanks, we appreciate the PR. Although, I have used v3 while it was in beta and had no issues. I am with Cory on leaving this open for a little while to ensure it gets proper eyes on it.

@kwelch
Copy link
Collaborator

kwelch commented Mar 30, 2017

Is there anything holding this PR up?

@kwelch kwelch mentioned this pull request Mar 30, 2017
Closed
@coryhouse coryhouse merged commit 3c21567 into coryhouse:master Mar 30, 2017
@coryhouse
Copy link
Owner

Ah, forgot to merge this after it was updated. Merged! Thanks @oshalygin! 👍

@oshalygin
Copy link
Contributor Author

Thanks gents, love this starter btw :)

@oshalygin oshalygin mentioned this pull request Apr 2, 2017
Closed
TobiahRex pushed a commit to TobiahRex/react-slingshot that referenced this pull request Jun 8, 2017
@oshalygin @TobiahRex
Add react hot loader 3 (coryhouse#392)
82f1ee4 
- This commit wires up react-hot-loader 3 to
  be used in the application.  There are numerous
  benefits to the latest release, all of which
  can be seen at https://github.com/gaearon/react-hot-loader
- Note that the specific implementation around
  wrapping in a Root component is part of how
  react-hot-loader 3 needs to be configured.
- Note that the package is brought in as a
  dependency, not a dev dependency because of
  how it is switched at runtime or not.
- More information on the migration can be
  viewed at:
https://github.com/gaearon/react-hot-loader/tree/master/docs\#migration-to-30

Related: coryhouse#216
coryhouse pushed a commit that referenced this pull request Sep 3, 2017
@TobiahRex @coryhouse
Implement React Router@4.0.0 (#393)
134b5f9 
* removed react-router form package.json & installed react-router-dom.

* completed index.src refactor.

* re-implemented App.js

* added notes to import changes & App.js

* modified middleware array @ store/configureStore.js

* updated yarn.lock file with fresh `yarn` command.

* updated master branch.

* upgraded react-hot-reloader to @3.0.0-beta.6

* removed extra "document.getElementById(app) from Root.js

* moved react-router-redux to regular "dependencies" from "dev-dependencies"

* cleaned Root.propTypes.

* cleaned Root.js & App.js per @oshalygin feedback.

* replaced .gitignore comment.

* removed react-router form package.json & installed react-router-dom.

* completed index.src refactor.

* re-implemented App.js

* added notes to import changes & App.js

* Update FAQ.md

fixed header on FAQ page

* Update README.md

fixed a few headers improperly declared

* modified middleware array @ store/configureStore.js

* Add react hot loader 3 (#392)

- This commit wires up react-hot-loader 3 to
  be used in the application.  There are numerous
  benefits to the latest release, all of which
  can be seen at https://github.com/gaearon/react-hot-loader
- Note that the specific implementation around
  wrapping in a Root component is part of how
  react-hot-loader 3 needs to be configured.
- Note that the package is brought in as a
  dependency, not a dev dependency because of
  how it is switched at runtime or not.
- More information on the migration can be
  viewed at:
https://github.com/gaearon/react-hot-loader/tree/master/docs\#migration-to-30

Related: #216

* Updated react-hot-loader to correct package version. (#401)

* Add item to check if issues

* updated yarn.lock file with fresh `yarn` command.

* Fix formatting (#403)

* updated master branch.

* upgraded react-hot-reloader to @3.0.0-beta.6

* removed extra "document.getElementById(app) from Root.js

* moved react-router-redux to regular "dependencies" from "dev-dependencies"

* cleaned Root.js & App.js per @oshalygin feedback.

* replaced .gitignore comment.

* clean index.js

* Add CONTRIBUTE.md (#431)

* Add

* Updated yarn lock using upgrade

* Rename

* Update

* Upgrade to webpack 3

* Update yarn lock

* Update snapshot

* Set prod env when analyzing bundle

* Add jest-cli as dependency

* Revert PR #450 (#451)

Removed change that removed additional dashes in npm test scripts in favor of adding jest-cli as a devDep.

This commit instead focusses on issue 2 from #449 where setupPrompts.js had a bug that caused start script to fail.

* Issue #449 fix (#450)

* Issue #449 fix

Issue 1:
Removed extra dashes located in the package.json test scripts that cause start script to fail.

Issue 2:
Removed escape characters found in setupPrompts.js which cause linting to fail, thus breaking start script.

* Jest fix

Re-added previously removed dashes from test scripts in package.json that caused start script to fail. Instead, @coryhouse added in jest-cli as a dev-dep which resolves the issue.

* Enhance babel env config to transpile for IE9+ (#452)

* Fix for jest handling of static assets when running tests. See: (#457)

jestjs/jest#2663 (comment)

* Added tips for npm run lint and build errors (#151) (#460)

* pushing changes from upstream fetch.

* updated from rebase.

* modified package versions in package.json & created new build.

* update package.json

* fixed conflicts with upstream master.

* cleaned up PropTypes validations - react-router-redux throwing PropTypes error.

* comment spell check & de-console on Root.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwelch kwelch kwelch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@oshalygin @coveralls @coryhouse @kwelch