Ensure that TS can run in a browser by checking for a process obj before using it in the perf logger

[orta]

Edit by @DanielRosenwasser: Fixes #33142

When I updated the TS for the web via Monaco-typescript, it would crash on using process in a web environment.

In microsoft/monaco-typescript#36 I added a workaround for that project, but it might be worth putting this also in a 3.6 branch if we expect to have more patch releases anytime soon. Then other projects using TS in a browser don't have to apply the same workarounds.

/cc @mrcrane

[andrewbranch]

I think this is ok in a pinch, but it actually feels a bit weird to me that this module has observable side effects upon being loaded. @mrcrane, I’m not too familiar with your ETW work, but could this log statement be wrapped in an exported function instead of evaluated immediately? Or at least be predicated upon the existence of etwModule?

[mrcrane]

Logging is predicated on the existence of etwModule through the use of nullLogger on line 40.

However there seems to be a problem where we are still evaluating the parameter to the logging function even though nullLogger will ignore it. I'm not sure of the best way to solve that problem in a concise way for all of the perfLogger.log statements that are littered throughout the code base. Is that the problem you are trying to solve?

This particular log statement does not really need to be called immediately from within this module. Instead of wrapping it in an exported function we could just simply move the log statement somewhere else if that would be more appropriate. But it would still need a fix like the one in this PR. Alternatively we could come up with a way to guard the parameter from being evaluated, which we would want to apply to all log statements throughout the code base.

[andrewbranch]

The real problem is that process can’t be referenced without checking it, because some parts of TypeScript need to be able to run in the browser. But the fact that this runs something (even though in most contexts it’s a noop) on load is a little unusual. I would expect a startup message to go, perhaps, executeCommandLine in tsc.ts.

As-is, you could get some strange effects by using TypeScript’s Node API. Imagine you’re working in a Node project in which you have @microsoft/typescript-etw installed, and you do something like

import * as ts from 'typescript';

export async function runSomething() {
  await someLongRunningSetupThing();
  doSomethingWithTypeScript(ts);
}

In this program, the log happens immediately after the VM parses this source file, even though you haven’t started doing anything with TypeScript yet. Your program may never do something with TypeScript, and yet the logger will say “Starting TypeScript... with command line” and then spit out the command line arguments that were given to your program, not even to TypeScript. So yeah, it’s easy to work around the process problem here, but it drew my attention to the fact that this might not be the right place for that log statement in the place.

[mrcrane]

Ok I see what you're getting at now and I agree. I wasn't initially aware of those other ways to load TypeScript.

Actually our primary use case for this perf logging is around tsserver so we would like to capture tsserver args, but having them available for tsc could be useful as well. In any case this can be resolved by removing this perfLogger.log statement from this file and essentially pasting it into those other entrypoints where we want to capture the args. It does not require any local variables so it should be fully moveable. That would certainly be a more sane solution.

[andrewbranch]

+1 for targeting 3.6, cc @DanielRosenwasser

[andrewbranch]

@mrcrane Orta is out right now, so I’m planning to just merge this as-is, but it would be great if we could follow up and move the log statement as we discussed in the thread. I opened #33246 to track—GitHub wouldn’t let me assign you, so I assigned myself, but if you get around to moving it, that would be swell. Feel free to ping me here or on Teams if you have questions.

[andrewbranch]

@typescript-bot cherry-pick this into release-3.6

[typescript-bot]

Hey @andrewbranch, I've opened #33247 for you.