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

Directly using process.argv in typescript.js #33142

Closed
Bobris opened this issue Aug 29, 2019 · 5 comments · Fixed by #33141
Closed

Directly using process.argv in typescript.js #33142

Bobris opened this issue Aug 29, 2019 · 5 comments · Fixed by #33141
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@Bobris
Copy link

Bobris commented Aug 29, 2019

TypeScript Version: 3.6.2 (offending code is still in repo when writing this)

Search Terms:
process argv etw

Code

perfLogger.logInfoEvent(`Starting TypeScript v${versionMajorMinor} with command line: ${JSON.stringify(process.argv)}`);

    perfLogger.logInfoEvent(`Starting TypeScript v${versionMajorMinor} with command line: ${JSON.stringify(process.argv)}`);

Expected behavior:
Should not use process directly.

Actual behavior:
It crashes in hosts which does not have process like browsers or any other than nodejs.

Bobris added a commit to bobril/bbcore that referenced this issue Aug 29, 2019
@orta orta added the Fix Available A PR has been opened for this issue label Aug 30, 2019
@orta orta self-assigned this Aug 30, 2019
@orta
Copy link
Contributor

orta commented Aug 30, 2019

Thanks - this has a PR #33141 open

@kitsonk
Copy link
Contributor

kitsonk commented Sep 6, 2019

Just ran into this when trying to update Deno to 3.6.

Looking at the fix available, it still assumes that every runtime with have process.argv as the way to get the command line arguments or it is a browser. Everything else in TypeScript like this is generally abstracted away from that through CompilerHost or System. Wouldn't it make sense here too? Or this code should detect that it wasn't invoked via tsc.

This fix, IMO, is plastering over the cracks.

@dsherret
Copy link
Contributor

dsherret commented Sep 6, 2019

@kitsonk agree.

Also, might be better to move this to log in the cli part (executeCommandLine in tsc.ts? Seems like that uses ts.sys.args) rather than when typescript is imported. Or is there a reason for this to log the process command line arguments even when not launched from tsc?

@andrewbranch
Copy link
Member

Yes, this fix is plastering over the cracks. As I mentioned in the PR, I plastered over the cracks because we’re about to release 3.6.3 and the other people involved were out of town. Knowing it’s not the fix we want, I opened #33246.

@fatcerberus
Copy link

Yeah, I’m also going to need to a proper fix for this in order to integrate TypeScript in miniSphere, which also doesn’t implement process.argv. For now I’m using 3.5, which doesn’t have the issue.

Definitely seems like typescript.js shouldn’t be responsible for this in the first place though - that’s the part that’s meant to be embedded so it’s not guaranteed that the concept of “command line arguments” even makes sense in a given environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
7 participants