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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

justgage
Copy link

Here's my second pass at the documentation for Create React App. A better version of #49 hopefully 😅

Here's my second pass at the documentation for Create React App. Hopefully you like this better.
@coveralls
Copy link

coveralls commented Mar 30, 2019

Coverage Status

Coverage remained the same at 97.978% when pulling fabecad on justgage:patch-2 into ad64578 on glennsl:master.

Copy link
Owner

@glennsl glennsl left a 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.

README.md Outdated Show resolved Hide resolved
@@ -63,15 +63,59 @@ Then add `__tests__` to `sources` in your `bsconfig.json`:
```js
"sources": [
{
"dir": "src"
"dir": "app",
Copy link
Owner

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
Copy link
Owner

@glennsl glennsl Apr 3, 2019

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)
Copy link
Owner

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
Copy link
Owner

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?

@peterpme
Copy link

peterpme commented Oct 11, 2019

👋 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
}
  • If I don't include a top level __tests__ folder, bs yells at me
  • If I don't put my tests in the top level __tests__ folder, open Jest can't be found.

Opinions aside, is this supposed to be the case? Do all my tests need to be in a top level test folder?

@glennsl
Copy link
Owner

glennsl commented Oct 12, 2019

Hi @peterpme :)

If I don't include a top level tests folder, bs yells at me

This seems like reasonable behaviour from bsb. It indicates there is a problem with your setup.

If I don't put my tests in the top level tests folder, open Jest can't be found.

I assume that's because you have bs-jest as a bs-dev-dependency, as you should, and the places you put your tests aren't in a source directory that has "type": "dev".

Do all my tests need to be in a top level test folder?

No, but they need to be in a folder marked as "type": "dev" in bsconfig.json. But from what I understand of CRA, they can't be in a top level folder because CRA runs jest in the src folder, not in the root folder. So I think you need something like this:

"sources": [{
  "dir": "src/app",
  "subdirs": true
}, {
  "dir": "src/__tests__",
  "type": "dev",
  "subdirs": true
}]

And then you have to put all the tests in src/__tests__.

It's also possible to have separate __tests__ folders alongside your app code, but then you ahve to turn off subdirs and list every folder explicitly in bsconfig.json, which is most likely not what you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants