Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Bad command-line for debugDiagnosticSeverity does not crashes Prepack anymore #2545

Closed
wants to merge 4 commits into from

Conversation

giftkugel
Copy link
Contributor

This can be a possible fix for #2509

I have removed the invariant method call and added a value check as used for the invariantMode option.

node lib/prepack-cli.js --debugDiagnosticSeverity foo

will now generate

Unsupported debugDiagnosticSeverity: foo

@giftkugel giftkugel changed the title Fix for https://github.com/facebook/prepack/issues/2509 Bad command-line for debugDiagnosticSeverity does not crashes Prepack anymore Sep 11, 2018
Copy link
Contributor

@NTillmann NTillmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much!

@giftkugel
Copy link
Contributor Author

You're welcome, but I am not happy, that I am not passing some checks ...
First the yarn prettier-all check failed because I didn't know about it.
Now I am failing the flow version; flow check check. 😞

But the old code was passing arg also directly to debuggerConfigArgs.diagnosticSeverity which is Severity type. Did the old code pass the flow type check?

arg === "FatalError" || arg === "RecoverableError" || arg === "Warning" || arg === "Information",
`Invalid debugger diagnostic severity: ${arg}`
);
if (!DiagnosticSeverityValues.includes(arg)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flow does not understand this test well enough to be know that the assignment in line 307 below is type safe. You can fix that by moving the deleted invariant to just before line 307.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not necessary to insert that call of the invariant method again, as I wanted to omit a method call which does not have any function.

I have added a flow type cast to the line and that fixed the flow type check.

src/prepack-cli.js Outdated Show resolved Hide resolved
Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hermanventer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

4 participants