-
Notifications
You must be signed in to change notification settings - Fork 5
Add support for create-react-app using Typescript #48
Add support for create-react-app using Typescript #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I've got a small remark. Namely that TS should be uppercase, like DB
and MQ
(but unlike Xml
for example).
src/JestConfigEditor.ts
Outdated
@@ -3,6 +3,7 @@ import { Config, ConfigEditor } from 'stryker-api/config'; | |||
import JestConfigLoader from './configLoaders/JestConfigLoader'; | |||
import DefaultJestConfigLoader from './configLoaders/DefaultJestConfigLoader'; | |||
import ReactScriptsJestConfigLoader from './configLoaders/ReactScriptsJestConfigLoader'; | |||
import ReactScriptsTsJestConfigLoader from './configLoaders/ReactScriptsTsJestConfigLoader'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS
should be uppercase right? Like DB
and MQ
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, and to be honest, I don't really care either. If you prefer TS
, TS
it will be ;-).
} else { | ||
JestTestAdapterFactory.log.debug(`Detected Jest between above 22.0.0`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log message is confusing to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of a left-over from back when Jest 19 till 22 was supported. I'll be remove it.
README.md
Outdated
**Note:** When neither of the options are specified it will use the jest configuration in your "package.json". \ | ||
**Note:** the `project` option is ignored when the `config` option is specified. \ | ||
**Note:** When neither of the options are specified it will use the Jest configuration in your "package.json". \ | ||
**Note:** the `project` option is ignored when the `config` option is specified. | ||
**Note:** Stryker currently only works for CRA-projects that have not been _ejected_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does ejected mean? Is it a React thing? Maybe we can add a link to where the concept is explained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the CRA documentation. This particular remark was already there since #46.
Basically, CRA allows you to "eject", which
will copy all the configuration files and the transitive dependencies (Webpack, Babel, ESLint, etc) right into your project so you have full control over them. All of the commands except eject will still work, but they will point to the copied scripts so you can tweak them. At this point you’re on your own.
"@types/node": "^9.6.4", | ||
"@types/react": "^16.3.9", | ||
"@types/react-dom": "^16.0.5", | ||
"typescript": "^2.8.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These dependencies aren't actually used right? Maybe delete them so they are not accidentally installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use them, and they aren't installed either. I'd rather keep them here, because tests can also double as documentation - people might want to take a look there to see how things are supposed to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
As noted in #40, the Jest runner doesn't resolve Jest configuration properly when you use create-react-app with TypeScript. This PR resolves that.
Breaking change: it drops support for Jest below 22.x.