-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add Create React App Section, clarify tradeoffs #50
base: master
Are you sure you want to change the base?
Conversation
Here's my second pass at the documentation for Create React App. Hopefully you like this better.
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 think src/app
should be the generally recommended project structure unless that's the consensus in the rest of the community. For most people just adding a __tests__
directory will be sufficient and has much less friction.
I also don't see a need for the CRA section if the generally recommended approach works well. But since I think that should be reverted, I'd rather replace the contents of the CRA section with just a recommendation to use src/app
along with src/__tests__
in dev mode.
@@ -63,15 +63,59 @@ Then add `__tests__` to `sources` in your `bsconfig.json`: | |||
```js | |||
"sources": [ | |||
{ | |||
"dir": "src" | |||
"dir": "app", |
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 should be "src"
, not "app"
@@ -63,15 +63,59 @@ Then add `__tests__` to `sources` in your `bsconfig.json`: | |||
```js | |||
"sources": [ | |||
{ | |||
"dir": "src" | |||
"dir": "app", | |||
"subdirs": true |
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.
Having this here is a bit opinionated I think, and kind of implies it's necessary for bs-jest to work, when it really isn't.
README.md | ||
|
||
|
||
(NOTE: no .re files in the root of src, if you put them there they will not be built) |
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 also seems unnecessary here as it's unrelated to the workings of bs-jest. It's good advice, but it should be included in the bucklescript docs instead.
(NOTE: no .re files in the root of src, if you put them there they will not be built) | ||
``` | ||
|
||
### For Create React App |
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 still don't see the content of this section as good advice. Why not recommend src/app
instead, and avoid all the warnings and such?
👋 hey, I'm in the process of using create-react-app with bs-jest and i'd like to revisit/hash out issues i've personally faced. i do agree with @glennsl's feedback - we include just the bare minimum required to get create-react-app working in this scenario. Here are some of my high level questions: Top level tests vs. src//tests** To make sure we're on the same page, I have this in my bsconfig.json {
"dir": "__tests__",
"type": "dev",
"subdris": true
}
Opinions aside, is this supposed to be the case? Do all my tests need to be in a top level test folder? |
Hi @peterpme :)
This seems like reasonable behaviour from
I assume that's because you have
No, but they need to be in a folder marked as "sources": [{
"dir": "src/app",
"subdirs": true
}, {
"dir": "src/__tests__",
"type": "dev",
"subdirs": true
}] And then you have to put all the tests in It's also possible to have separate |
Here's my second pass at the documentation for Create React App. A better version of #49 hopefully 😅