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

Compile with --noImplicitThis #9580

Merged
merged 6 commits into from
Jul 11, 2016
Merged

Compile with --noImplicitThis #9580

merged 6 commits into from
Jul 11, 2016

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Jul 8, 2016

  1. Add --noImplicitThis to various tsconfig.json (gulp uses tsconfig).
  2. Add --noImplicitThis to Jakefile.
  3. Add annotations where needed.
  4. Add workaround to shims.ts, which uses toplevel this.

Also, fixes #9608

1. Add to various tsconfig.json
2. Add to Jakefile
3. Add annotations where needed.
4. Add workaround to shims.ts, which uses toplevel `this`.
@sandersn
Copy link
Member Author

sandersn commented Jul 8, 2016

The CI build pointed out that I failed to update the harness tsconfig.json -- Jake caught it because it uses the command line parameter instead of tsconfig.json

@@ -1237,20 +1237,20 @@ namespace ts {
getSignatureConstructor(): new (checker: TypeChecker) => Signature;
}

function Symbol(flags: SymbolFlags, name: string) {
function Symbol(this: Symbol, flags: SymbolFlags, name: string) {
Copy link
Member

Choose a reason for hiding this comment

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

@rakatyal just remembered your tslint rule would have issues with this because it can't distinguish between ts.Symbol and Symbol

@sandersn
Copy link
Member Author

I tested that the shims workaround to obtain global this does in fact work with ChakraHost.

@sandersn
Copy link
Member Author

@weswigham I updated Gulpfile.ts with some defaults for tasks that use gulp-typescript's settings object instead of a tsconfig. Unfortunately, the one I wanted, --noImplicitThis isn't available since it's 2.0-only of course. Anyway, you might want to double-check those changes.

@weswigham
Copy link
Member

There's a patch at the top of the Gulpfile adding in options gulp-typescript doesn't yet support. You should just add more to that (since it just passes thru ones it doesn't need to read).

@sandersn
Copy link
Member Author

Got it -- I added them.

@weswigham
Copy link
Member

Do you want to add pretty: true to the tsconfigs while you're editing them/remembering to add it to the other targets?

@@ -2081,7 +2081,7 @@ namespace ts.server {
const walkFns = {
goSubtree: true,
done: false,
leaf: function (relativeStart: number, relativeLength: number, ll: LineLeaf) {
leaf: function (this: ILineIndexWalker, relativeStart: number, relativeLength: number, ll: LineLeaf) {
if (!f(ll, relativeStart, relativeLength)) {
this.done = true;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of giving this function a this type, can you not just replace this with walkFns (since it is just an object literal)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@sandersn
Copy link
Member Author

Good idea, the output doesn't look much prettier than before, so I might be doing something wrong.

@weswigham
Copy link
Member

gulp-typescript uses an LS to compile, so pretty may not do anything until gulp-typescript adds support for it.

Also, this PR fixes #9608.

@sandersn
Copy link
Member Author

#9608 also mentions --preserveConstEnums --inlineSourceMap --inlineSources. I think the gulpfile already handles the last two, but what about preserveConstEnums?

@sandersn
Copy link
Member Author

Never mind, the code's already there for preserveConstEnums. I just missed it.

@@ -1398,7 +1398,7 @@ namespace ts {
return noDiagnosticsTypeChecker || (noDiagnosticsTypeChecker = createTypeChecker(program, /*produceDiagnostics:*/ false));
}

function emit(sourceFile?: SourceFile, writeFileCallback?: WriteFileCallback, cancellationToken?: CancellationToken): EmitResult {
function emit(this: Program, sourceFile?: SourceFile, writeFileCallback?: WriteFileCallback, cancellationToken?: CancellationToken): EmitResult {
return runWithCancellationToken(() => emitWorker(this, sourceFile, writeFileCallback, cancellationToken));
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to exchange this instance of this for program (which should be in scope)? Or is that excessive?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmmmm...there's too much space between program and emit. I'm going to leave it. I don't think there's an easy solution, besides refactoring everything to use method syntax, which also has some bad side effects.

@weswigham
Copy link
Member

👍 With your discretion on my last comment.

@sandersn sandersn merged commit f19844f into master Jul 11, 2016
@sandersn sandersn deleted the compile-with-noImplicitThis branch July 11, 2016 22:25
@guncha
Copy link

guncha commented Aug 8, 2016

Please consider using an alternative to (new Function("return this"))() in src/services/shims.ts:19 since it breaks in restricted environments like Atom that have Content Security Policy set to disallow eval-like behavior.

As far as I can understand, this is necessary to get the global object value without using this to satisfy the Typescript type checker. Perhaps having a non-strict outside function return this or checking for typeof window or typeof global would do the job.

@sandersn
Copy link
Member Author

@guncha would (function (this: any) { return this; })() be all right?
(The this: any parameter silences the error that this PR enabled.)

@sandersn
Copy link
Member Author

A PR is up at #11265

@guncha
Copy link

guncha commented Sep 30, 2016

@sandersn it would fix the issue I've been seeing with Atom, thank you.

@sandersn
Copy link
Member Author

Glad to hear it. This fix will ship with Typescript 2.1

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gulpfile is missing some compiler settings
5 participants