Open
Conversation
Owner
|
@jacopotarantino Thanks for your PR and sorry so late, just now seeing this. Reviewing... |
mcarlucci
requested changes
May 9, 2019
| import ReactPrecacheImg from './react-precache-img'; | ||
|
|
||
| export { ReactPrecacheImg }; | ||
| export default ReactPrecacheImg; |
Owner
There was a problem hiding this comment.
Would it make more sense to update the function name in src/react-precache-img.js from PreCacheImg to ReactPrecacheImg instead?
| "eslint-plugin-jsx-a11y": "^3.0.2", | ||
| "eslint-plugin-react": "^6.9.0", | ||
| "jest-cli": "^18.1.0", | ||
| "prop-types": "^15.7.2", |
Owner
There was a problem hiding this comment.
This should be moved to dependencies
| import ReactDOM from 'react-dom'; | ||
| import TestUtils from 'react-addons-test-utils'; | ||
| import ReactPreCacheImgDemo from '../src/ReactPreCacheImgDemo.jsx' | ||
| import renderer from 'react-test-renderer'; |
Owner
There was a problem hiding this comment.
Getting the following failures when running npm run test:
| ~/projects/react-precache-img @ MacBook-Pro (mcarlucci)
| => npm run test
> react-precache-img@2.0.6 test /Users/mcarlucci/projects/react-precache-img
> BABEL_JEST_STAGE=0 jest
PASS __tests__/index-test.js
FAIL __tests__/ReactPreCacheImg-test.js
● Test suite failed to run
TypeError: Cannot read property 'hasOwnProperty' of undefined
at node_modules/react-test-renderer/cjs/react-test-renderer.development.js:151:27
at Object.<anonymous> (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:13241:5)
at Object.<anonymous> (node_modules/react-test-renderer/index.js:6:20)
at Object.<anonymous> (__tests__/ReactPreCacheImg-test.js:4:52)
at process._tickCallback (internal/process/next_tick.js:68:7)
FAIL __tests__/ReactPreCacheImgDemo-test.js
● Test suite failed to run
TypeError: Cannot read property 'hasOwnProperty' of undefined
at node_modules/react-test-renderer/cjs/react-test-renderer.development.js:151:27
at Object.<anonymous> (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:13241:5)
at Object.<anonymous> (node_modules/react-test-renderer/index.js:6:20)
at Object.<anonymous> (__tests__/ReactPreCacheImgDemo-test.js:4:52)
at process._tickCallback (internal/process/next_tick.js:68:7)
Test Suites: 2 failed, 1 passed, 3 total
Tests: 1 passed, 1 total
Snapshots: 0 total
Time: 1.071s
Ran all test suites.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! react-precache-img@2.0.6 test: `BABEL_JEST_STAGE=0 jest`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the react-precache-img@2.0.6 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR! /Users/mcarlucci/.npm/_logs/2019-05-09T05_40_55_956Z-debug.log
| "semi": true, | ||
| "singleQuote": true, | ||
| "arrowParens": "always" | ||
| } |
Owner
There was a problem hiding this comment.
Is this file an oversight? I don't see any packages installed or npm scripts that would leverage this file. I'm certainly not opposed to adding it, although eslint already supports prettier formatting that can be installed using the ESLint ecosystem. See https://prettier.io/docs/en/integrating-with-linters.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This work fixes an issue where propTypes was specifying
arrayOf.isRequiredwhere#arrayOfis actually a method and should have been fed an argument as such:arrayOf(propTypes.string).isRequired.While working on this I also took the liberty of updating some tests and imports to further enhance future compatibility.