Investigate what happened to unhandled rejection

[Timer]

#4193

[bradfordlemley]

FYI, according to this ci run, the lint error was:

/home/travis/build/facebook/create-react-app/packages/react-error-overlay/src/utils/getStackFrames.js
16:3 warning 'unhandledRejection' is assigned a value but never used no-unused-vars

[iansu]

I think the question here is what changed that made that error appear?

[bradfordlemley]

Yeah, just wanted to note the error here to hopefully save someone some time figuring out why it appeared :) (as in ... obviously, it's a valid error, but why did it just start appearing?)

[NMinhNguyen]

Could it be because of https://github.com/facebook/create-react-app/pull/4187/files? https://eslint.org/docs/rules/no-unused-vars says that

By default this rule is enabled with all option for variables and after-used for arguments.

However, that PR uses the all option for arguments. I'm gonna try to see if changing it to the default option instead makes the error go away. I personally think that after-used is a reasonable option for arguments.

UPDATE:

I think I've figured out what's going on. eslint/eslint#8040 (comment) seems to confirm that the previous code should've passed eslint.

function getStackFrames(
  error: Error,
  unhandledRejection: boolean = false,
  contextSize: number = 3
): Promise<StackFrame[] | null> {
  // ...
}

However, eslint/eslint#8459 seems to have broken it, because I took the current master, reverted the commit (including the new test cases), added a new test case similar to the issue we're having, and it passed (and it does fail on master, as expected):

NMinhNguyen/eslint@master...NMinhNguyen:no-unused-vars-repro-after-used-default-fn-params#diff-982d705c981c706fd513f006b18166c5R87

I'll try to raise an issue on eslint's issue tracker and try to prepare a PR as well, but realistically I probably won't get round to doing all of this until the weekend.

That being said, I think that for CRA it still makes sense not to error on all unused arguments. If you change the eslint rule to args: "after-used",, and remove the default parameter from thegetStackFrames function, then eslint doesn't complain. Also makes me realise that changing this setting is probably a breaking change and would need to be mentioned as such in the changelog.

 function getStackFrames(
   error: Error,
-  unhandledRejection: boolean = false,
+  unhandledRejection: boolean,
   contextSize: number = 3

[Timer]

Sorry, this was not clear at all.
Unhandled rejections used to be handled differently than normal errors so that the overlay would designate that the error was from a Promise.
It seems now that this distinction is gone.

[Marcholio]

Is there any update on this, can this be closed?

[mrmckeb]

@Timer I'll close this one off as we're not sure if it's still relevant, but please let me know if it is.