🎨 add first class debugging support #2041

cdaringe · 2017-04-28T08:46:52Z

continuation of #1360. re-implemented per code review remarks, and conflicts resolved. needs 👀 re-review :).

problem statement

react-scripts does not offer ready-to-roll jest debugging.

solution

add it!

closes #594

test

checkout this feature branch
/path/to/this/branch/create-react-app/bin/create-react-app dummy-app
cd dummy-app
modify npm test to add a debug flag ^^
tell your editor to honor source maps
npm test
connect the remote debugger
go.

cdaringe · 2017-04-28T16:58:48Z

packages/react-scripts/bin/react-scripts.js

+    var scriptFilename = require.resolve('../scripts/' + script);
+    var result = spawn.sync('node', getArgs(scriptFilename), {
+      stdio: 'inherit',
+    });


it took me a while to figure out what was going on here--the husky git hook lint script kept changing the format out from under me!

🙈

also, do we not already pass args in via [require.resolve('../scripts/' + script)].concat(args)?

i dont follow. are you suggesting just passing script to getArgs(...)? that is, getArgs(script), and move that path resolution line into the other file?

So we already pass arguments through, just not to Node. I didn't realize the flags had to come first, sorry.

They're passed to the script.

right right right. if desired, we can annotate if you think its fitting somewhere, or rename the fn's to be more communicative

cdaringe · 2017-04-28T19:26:07Z

packages/react-scripts/scripts/utils/debugArgs.js

+const DEBUG_FLAGS = [
+  /^debug$/,
+  /^--debug$/,
+  /^--debug-brk(=\d+)?$/,


I believe we shouldn't respect --debug and --debug-brk

are you sure? i'm not convinced we shouldn't support it!

They're going away in Node 8 iirc.

cdaringe · 2017-04-29T17:56:58Z

back at cha

This reverts commit 01ab6c9. See jestjs/jest#1652 for more. --inspect isn't honored reliably in jest ATM.

cdaringe · 2017-05-01T04:23:24Z

good news! debugging is working reliably! good enough where i think we should ship, after any polish requested is applied.

bad news, --inspect* is no bueno. essentially jestjs/jest#1652 prevents us from using JSDom in jest & the inspector protocol simultaneously. i'm not enough of a jest+JSDOM wizard to figure that out.

the code has been annotated, and --debug-brk still works great :), even in watch mode!
- note, jest in watch mode in master is really, painfully slow on OSX for whatever reason. unrelated to this PR, but woof!

~~i'm not sure when react-scripts is targeting another release, but i would really like to be able to bump my projects and start debugging asap!~~ 0.10.0 go team ⚽️

TheTFo · 2017-05-01T18:01:25Z

When can we expect a release including this issue?

cdaringe · 2017-05-02T00:05:45Z

BTW, I think the build needs to be kicked started again. I don't think my pathes are the source of failure. An extra set of 👀 would be appreciated

Timer · 2017-05-08T05:27:47Z

@cdaringe I don't want to leave this hanging, but I don't know if we're going to ship this until jestjs/jest#1652 is resolved.
I feel very uncomfortable relying on a flag that's already deprecated 😞; are there workarounds we can implement to get inspect working?

gaearon · 2017-05-08T14:50:51Z

I’m going to push this back to 0.11 so we can have the time to figure this out.

cdaringe · 2017-05-08T21:54:49Z

@Timer @gaearon,

why is node's flag deprecation a threat? i don't see any risk here. we only add value/capability in this PR. there's zero loss to stakeholders. we can rev react-scripts later when the jest team gets this sorted out.
- i do not think there's a hack to make --inspect work in the interim, unfortunately :(
i'm not sure what more we need to figure out. there's a known open issue against our dependency stack, but there is a reliable, non-hacky workaround--just limit usage to the older, but well tested & supported protocol.

debug capability, IMHO, is super important, & this is the 2nd time I've implemented it. i'm not upset, but i do not want to have to do it a 3rd time. can you speak more to why you feel we shouldn't get this in sooner?

caiobalthazar · 2017-05-08T22:34:48Z

I strongly agree with @cdaringe, this is very important for productivity and one of the worst parts of CRA testing.

TheTFo · 2017-05-09T07:50:30Z

I agree, with @cdaringe also. My team needs this capability as soon as possible.

…

On Mon, May 8, 2017, 6:35 PM Caio Balthazar ***@***.***> wrote: I strongly agree with @cdaringe <https://github.com/cdaringe>, this is very important for productivity and one of the worst parts of CRA testing. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2041 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgWoPOhKMtJexaWThVfx5D9uuBG-fvlks5r35iUgaJpZM4NLNqS> .

TheTFo · 2017-05-25T12:07:24Z

jestjs/jest#1652 has been closed with no action taken.

gaearon · 2017-05-25T12:48:44Z

There is nothing Jest can do to fix it. Please express your support for nodejs/node#7593.

gaearon · 2017-05-26T21:31:23Z

At this point we just need somebody to help push this through.
nodejs/node#9272 (comment)

Any volunteers?

Timer · 2017-05-26T22:07:03Z

@cdaringe sorry I didn't respond earlier

why is node's flag deprecation a threat? i don't see any risk here. we only add value/capability in this PR. there's zero loss to stakeholders. we can rev react-scripts later when the jest team gets this sorted out.

It's not so much that it's a threat, but if it becomes something we support, it's turns into our job to maintain that feature going forward.
When Node 8 is released, it is highly likely that --debug will not exist anymore and the only option will be --inspect (because it was removed from v8).

Jest doesn't work with --inspect, so we'd have to tell users to not upgrade their Node versions if they'd like to debug.

This puts us in a bad position because people will expect test debugging to work across all LTS versions of Node (as they should).
IMO, this is plunging us back into the era of JavaScript F***gue where environment matters and tools don't work reliably.

Secondly, Create React App is a toolchain focused at beginners first, but we still consider advanced users and try to accommodate their needs.
Teaching beginners how to debug using a deprecated method and then telling them they have to switch to --inspect (external of our control [based on their environment]) would be painful and cause a potentially poor/confusing experience.

debug capability, IMHO, is super important, & this is the 2nd time I've implemented it. i'm not upset, but i do not want to have to do it a 3rd time.

I'm really sorry that you feel your development efforts are being wasted.
We try really hard to incorporate as many changes as we can and try to ensure developers feel valued. (and we do value your contributions very much, really.)

We will use your work (and not waste it 😄) as a foundation for building out this feature in the future if you are unable to allocate time, but I hope you can understand our position.

I would feel comfortable adding support for --debug if we had a solid timeline and plan for Jest supporting --inspect (which is blocked by Node currently).

gaearon · 2017-05-26T22:17:24Z

I think any effort here should be directed towards unblocking Node. Let's just fix it there.

Timer · 2017-05-31T14:51:42Z

Just dropping this here, speculation was correct and --debug is dead:

Joes-MacBook-Pro:16s-website joe$ node --debug
(node:70606) [DEP0062] DeprecationWarning: `node --debug` and `node --debug-brk` are invalid. Please use `node --inspect` or `node --inspect-brk` instead.

Really looking forward to nodejs/node#7593 and nodejs/node#9272 (comment) progressing.

Debugging Jest is still broken. See facebook/create-react-app#2041

gaearon · 2017-06-01T12:59:49Z

Doesn’t seem like anybody wants to take ownership 😞 .

cdaringe · 2017-06-02T00:45:10Z

My support hath been cast!

avantgardnerio · 2017-08-07T09:37:32Z

This and various related issues have been open since fall 2016. It seems like debugging is a second class citizen in the react/jest/CRA community? I know some folks think if tests are so complicated you need to debug them, then you are doing it wrong, but I personally think a debugger (with good tests) is one of the most productivity enhancing tools of all times. It would give me great confidence in the react community if debugging was a first-class concern.

For starters, not creating profiles in memory or in an obscure temporary folder would be a good step towards making the transpiled code more obvious. Also, doing things in process vs launching a new process would help considerably.

Timer · 2017-08-07T10:45:36Z

We'd really like to implement this feature but Node does not support it. See nodejs/node#7593 and nodejs/node#9272 (comment).

If you can help implement the feature in Node, that'd be great!

lifeiscontent · 2017-08-14T07:10:27Z

@Timer can we move forward on this yet now that inspector has been merged in node?

jgoz · 2017-08-15T22:31:51Z

FYI, debugging Jest tests with inspector is now possible as of Node 8.4.0 (nodejs/node#14465).

avantgardnerio · 2017-08-16T01:41:29Z

@jgoz Thank you for mentioning that here! I'll upgrade and try it again. I'm very happy to see this fixed!

gaearon · 2017-08-16T17:19:43Z

This and various related issues have been open since fall 2016. It seems like debugging is a second class citizen in the react/jest/CRA community?

No. We were just as frustrated about this. Unfortunately we lacked the necessary knowledge (knowing C, understanding of how contexts work in Node and V8) to fix this. We are also glad the fix has landed! Sometimes a hard fix is just a hard fix, and not some intentional choice. :-)

I don't understand what you mean about profiles.

dchambers · 2017-08-16T20:39:54Z

Can any provide guidance as to how to debug Jest tests using Node.js 8.4.0? I've so far tried:

npm test -- --inspect-brk

and:

node --inspect-brk node_modules/jest/bin/jest.js -i

with no joy.

cdaringe · 2017-08-16T20:41:11Z

@dchambers, see #594 (comment). make sure you're using version 1.0.11+

dchambers · 2017-08-16T20:53:49Z

Thanks @cdaringe, upgrading 'react-scripts' to 1.0.11 then running ./node_modules/.bin/react-scripts --inspect --inspect-brk test --env=jsdom --runInBand did it for me 🎉

Timer · 2017-09-29T01:17:45Z

Closing this as support has been added in 1.0.11, thanks for all of your hard work!

TheTFo · 2017-10-22T00:02:37Z

Hey all,

I think this is broken. I'm having issues with debugging my thinks.

Here's repro branch

https://github.com/TheTFo/cria/tree/jest-debug?files=1

Attempt to debug tests from vscode launcher, placing a break point on the line indicated in comment in this file:

https://github.com/TheTFo/cria/blob/jest-debug/src/actions/todoActions.js

Breakpoints don't appear to hit the appropriate lines. Not sure what's causing it. I don't see transpiled code, just wierd breakpoints hits.

valoricDe · 2017-11-14T12:24:38Z

@TheTFo had the same problem. Comment #594 (comment) fixed it for me

facebook-github-bot added the CLA Signed label Apr 28, 2017

cdaringe mentioned this pull request Apr 28, 2017

feature/add-first-class-debugging-support-for-tests #1360

Closed

cdaringe force-pushed the feature/first-class-debugging branch from bf88ccd to 36244d6 Compare April 28, 2017 16:08

🎨 add first class debugging support

73c8729

cdaringe force-pushed the feature/first-class-debugging branch from 36244d6 to 73c8729 Compare April 28, 2017 16:11

cdaringe commented Apr 28, 2017

View reviewed changes

Timer added this to the 0.10.0 milestone Apr 28, 2017

cdaringe commented Apr 28, 2017

View reviewed changes

🔥 drop --debug*

01ab6c9

cdaringe added 2 commits April 30, 2017 21:04

Revert ":fire: drop --debug*" per https://tinyurl.com/k546ycf

f51b7d2

This reverts commit 01ab6c9. See jestjs/jest#1652 for more. --inspect isn't honored reliably in jest ATM.

fix(debug): disable inspect support per jestjs/jest#1652

3e5e51c

docs(test): add debug instructions

fdd4474

Timer mentioned this pull request May 5, 2017

1.0.0 Roadmap #2089

Closed

32 tasks

gaearon modified the milestones: 0.11.0, 0.10.0 May 8, 2017

lancefisher pushed a commit to lancefisher/cellularjs that referenced this pull request Jun 1, 2017

Trying to fix debugging

9e30c22

Debugging Jest is still broken. See facebook/create-react-app#2041

bmihelac mentioned this pull request Jun 6, 2017

Enable CLI debugger with Jest #691

Closed

Timer mentioned this pull request Jun 6, 2017

Debuging with node 8 #2478

Closed

Timer mentioned this pull request Aug 16, 2017

Document how to debug unit tests #594

Closed

Timer mentioned this pull request Aug 18, 2017

node --inspect appears to be broken or unsupported #2969

Closed

Timer closed this Sep 29, 2017

lock bot locked and limited conversation to collaborators Jan 20, 2019

🎨 add first class debugging support #2041

🎨 add first class debugging support #2041

Conversation

cdaringe commented Apr 28, 2017 • edited Loading

problem statement

solution

test

cdaringe Apr 28, 2017

Choose a reason for hiding this comment

Timer Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

cdaringe Apr 28, 2017

Choose a reason for hiding this comment

Timer Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

cdaringe Apr 29, 2017

Choose a reason for hiding this comment

cdaringe Apr 28, 2017

Choose a reason for hiding this comment

Timer Apr 28, 2017

Choose a reason for hiding this comment

cdaringe commented Apr 29, 2017

cdaringe commented May 1, 2017 • edited Loading

TheTFo commented May 1, 2017

cdaringe commented May 2, 2017

Timer commented May 8, 2017

gaearon commented May 8, 2017

cdaringe commented May 8, 2017 • edited Loading

caiobalthazar commented May 8, 2017

TheTFo commented May 9, 2017 via email

TheTFo commented May 25, 2017

gaearon commented May 25, 2017

gaearon commented May 26, 2017

Timer commented May 26, 2017 • edited Loading

gaearon commented May 26, 2017

Timer commented May 31, 2017

gaearon commented Jun 1, 2017

cdaringe commented Jun 2, 2017

avantgardnerio commented Aug 7, 2017 • edited Loading

Timer commented Aug 7, 2017

lifeiscontent commented Aug 14, 2017

jgoz commented Aug 15, 2017

avantgardnerio commented Aug 16, 2017

gaearon commented Aug 16, 2017

dchambers commented Aug 16, 2017

cdaringe commented Aug 16, 2017

dchambers commented Aug 16, 2017

Timer commented Sep 29, 2017

TheTFo commented Oct 22, 2017

valoricDe commented Nov 14, 2017

cdaringe commented Apr 28, 2017 •

edited

Loading

Timer Apr 28, 2017 •

edited

Loading

Timer Apr 28, 2017 •

edited

Loading

cdaringe commented May 1, 2017 •

edited

Loading

cdaringe commented May 8, 2017 •

edited

Loading

Timer commented May 26, 2017 •

edited

Loading

avantgardnerio commented Aug 7, 2017 •

edited

Loading