-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Issue #449 fix #450
Issue #449 fix #450
Conversation
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.
Hi @DaneTheory - Thanks for the PR! The changes for issue 2 look good, but the "extra dashes" aren't a bug, they're required to pass extra args to another script. What issue were you seeing with the scripts in package.json? Anyway, if you revert the package.json changes, I can merge this. |
Good morning @coryhouse. Awesome job with react-slingshot! I posted the output of the issue on the issue I posted for it. Added a ref link in the PR: #449 Removing the extra dashes fixed the issue on my end. But if you can't reproduce the bug from the info I posted I'll go ahead and remove the changes so we can get this merged. |
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.
Just checked your notes on #449. Added back in the dashes. Good catch on noticing jest-cli needed to be a devDep! Cheers. |
Thanks! Since there are only whitespace changes on package.json now, can you just revert it? |
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 coryhouse#449 where setupPrompts.js had a bug that caused start script to fail.
Are we good to merge this? Looks good to me. |
Was waiting for the revert on the unnecessary whitespace changes in package.json, but I suppose they're fine. Merging. Thanks for the reminder @kwelch! |
* 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
$ npm install npm ERR! A complete log of this run can be found in: (BAGAI MANA MENYELESAIKAN INI?) |
Ref: #449
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.