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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from Sep 4, 2019
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
206de25
Ensure that TS can run in a browser by checking for a process obj bef…
orta Aug 29, 2019
45227ee
Merge branch 'master' into allow_running_in_web
andrewbranch Sep 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/compiler/perfLogger.ts
View file
Open in desktop
Expand Up @@ -38,6 +38,6 @@ namespace ts {

/** Performance logger that will generate ETW events if possible */
export const perfLogger: PerfLogger = etwModule ? etwModule : nullLogger;

perfLogger.logInfoEvent(`Starting TypeScript v${versionMajorMinor} with command line: ${JSON.stringify(process.argv)}`);
const args = typeof process === "undefined" ? [] : process.argv;
perfLogger.logInfoEvent(`Starting TypeScript v${versionMajorMinor} with command line: ${JSON.stringify(args)}`);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}