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

[now-node] fix reportTSError exiting #3244

Merged
merged 1 commit into from
Nov 8, 2019

Conversation

igorklopov
Copy link
Contributor

This fixes abnormal process exit when there is an error like console.log(Math.round("42")); and compilerOptions.noEmitOnError is true in tsconfig.json. See https://github.com/zeit/now/blob/2dee810e74ff656cb36184656949268d2e4e038b/packages/now-node/src/typescript.ts#L307 and https://github.com/zeit/now/blob/2dee810e74ff656cb36184656949268d2e4e038b/packages/now-node/src/typescript.ts#L197

@styfle
Copy link
Member

styfle commented Nov 4, 2019

@igorklopov PRs should be submitted to the canary branch (not master)

@styfle
Copy link
Member

styfle commented Nov 4, 2019

Before this PR:

Using TypeScript 3.6.4 (local user-provided)
--
07:31:06 AM | Error: src/index.ts(1,24): error TS2345: Argument of type '"42"' is not assignable to parameter of type 'number'.
07:31:06 AM |  
07:31:06 AM | worker exited with code 1 and signal null
07:31:08 AM | done

After this PR:

Using TypeScript 3.6.4 (local user-provided)
--
05:32:51 PM | Error: src/index.ts(1,24): error TS2345: Argument of type '"42"' is not assignable to parameter of type 'number'.
05:32:51 PM | at createTSError (/zeit/67fa4190/.build-utils/.builder/node_modules/@now/node/dist/index.js:53668:16)
05:32:51 PM | at reportTSError (/zeit/67fa4190/.build-utils/.builder/node_modules/@now/node/dist/index.js:53674:23)
05:32:51 PM | at getOutputTypeCheck (/zeit/67fa4190/.build-utils/.builder/node_modules/@now/node/dist/index.js:53761:17)
05:32:51 PM | at compile (/zeit/67fa4190/.build-utils/.builder/node_modules/@now/node/dist/index.js:53839:40)
05:32:51 PM | at compileTypeScript (/zeit/67fa4190/.build-utils/.builder/node_modules/@now/node/dist/index.js:23305:31)
05:32:51 PM | at Job.readFile (/zeit/67fa4190/.build-utils/.builder/node_modules/@now/node/dist/index.js:23330:30)
05:32:51 PM | at Job.emitDependency (/zeit/67fa4190/.build-utils/.builder/node_modules/@now/node/dist/index.js:36460:25)
05:32:51 PM | at Promise.all.files.map.file (/zeit/67fa4190/.build-utils/.builder/node_modules/@now/node/dist/index.js:36288:18)
05:32:51 PM | at Array.map (<anonymous>)
05:32:51 PM | at Object.module.exports.module.exports [as default] (/zeit/67fa4190/.build-utils/.builder/node_modules/@now/node/dist/index.js:36284:27)
05:32:52 PM | worker exited with code 20 and signal null
05:32:54 PM | done

I'm not sure this is a better solution because the stack trace is not relevant to the user 🤔

@igorklopov
Copy link
Contributor Author

igorklopov commented Nov 6, 2019

Right. But in all cases (except this one with noEmitOnError) we anyways have unrelevant stack traces. This would be another issue to make proper error message (probably catching typescript exceptions somewhere at builder level). Meanwhile let's fix process.exit. @styfle

@styfle styfle changed the base branch from master to canary November 8, 2019 17:05
@styfle styfle force-pushed the now-node/fix-report-ts-error-exit branch from c28e3b8 to 26f5a36 Compare November 8, 2019 17:06
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

I changed the branch to canary and ran lint.

It looks like more was changed but its just line 197 that's relevant 👍

Thanks!

@kodiakhq kodiakhq bot merged commit a84b927 into canary Nov 8, 2019
@kodiakhq kodiakhq bot deleted the now-node/fix-report-ts-error-exit branch November 8, 2019 17:28
leo pushed a commit that referenced this pull request Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants