Compile with --noImplicitThis by sandersn · Pull Request #9580 · microsoft/TypeScript

sandersn · 2016-07-08T22:16:31Z

Add --noImplicitThis to various tsconfig.json (gulp uses tsconfig).
Add --noImplicitThis to Jakefile.
Add annotations where needed.
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 · 2016-07-08T22:26:15Z

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

DanielRosenwasser · 2016-07-09T00:34:45Z

    }

-    function Symbol(flags: SymbolFlags, name: string) {
+    function Symbol(this: Symbol, flags: SymbolFlags, name: string) {


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

sandersn · 2016-07-11T16:01:33Z

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

sandersn · 2016-07-11T16:33:45Z

@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 · 2016-07-11T17:07:16Z

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 · 2016-07-11T17:33:17Z

Got it -- I added them.

weswigham · 2016-07-11T17:52:10Z

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

weswigham · 2016-07-11T17:54:26Z

-                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;


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

sandersn · 2016-07-11T18:00:20Z

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

weswigham · 2016-07-11T18:04:26Z

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 · 2016-07-11T18:08:45Z

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

sandersn · 2016-07-11T18:09:18Z

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

weswigham · 2016-07-11T18:37:34Z


-        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));


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

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 · 2016-07-11T18:39:06Z

👍 With your discretion on my last comment.

guncha · 2016-08-08T18:35:39Z

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 · 2016-09-29T23:14:06Z

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

sandersn · 2016-09-29T23:18:00Z

A PR is up at #11265

guncha · 2016-09-30T15:16:31Z

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

sandersn · 2016-09-30T15:18:25Z

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

Compile with --noImplicitThis

b543074

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`.

msftclas added the cla-not-required label Jul 8, 2016

DanielRosenwasser reviewed Jul 9, 2016
View reviewed changes

Fix this in harness and improve gulpfile defaults

cf15e82

Add more default options to gulpfile

6d21cf6

weswigham reviewed Jul 11, 2016
View reviewed changes

Add --pretty to tsconfigs

a3d9a85

Avoid using this in object literals where possible

a138e63

weswigham reviewed Jul 11, 2016
View reviewed changes

Remove another use of this, in program.ts

6414a57

sandersn merged commit f19844f into master Jul 11, 2016

sandersn deleted the compile-with-noImplicitThis branch July 11, 2016 22:25

This was referenced Aug 8, 2016

Use loophole package to to fix #104 AtomLinter/linter-tslint#106

Merged

Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an al... AtomLinter/linter-tslint#104

Closed

sandersn mentioned this pull request Sep 29, 2016

Use new Function-less access to this for debugObjectHost #11265

Merged

sandersn mentioned this pull request Jun 18, 2018

Enable noImplicitThis webpack/webpack#7514

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

Uh oh!

Conversation

sandersn commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn commented Jul 8, 2016

Uh oh!

DanielRosenwasser Jul 9, 2016

Choose a reason for hiding this comment

Uh oh!

sandersn commented Jul 11, 2016

Uh oh!

sandersn commented Jul 11, 2016

Uh oh!

weswigham commented Jul 11, 2016

Uh oh!

sandersn commented Jul 11, 2016

Uh oh!

weswigham commented Jul 11, 2016

Uh oh!

weswigham Jul 11, 2016

Choose a reason for hiding this comment

Uh oh!

sandersn Jul 11, 2016

Choose a reason for hiding this comment

Uh oh!

sandersn commented Jul 11, 2016

Uh oh!

weswigham commented Jul 11, 2016

Uh oh!

sandersn commented Jul 11, 2016

Uh oh!

sandersn commented Jul 11, 2016

Uh oh!

weswigham Jul 11, 2016

Choose a reason for hiding this comment

Uh oh!

sandersn Jul 11, 2016

Choose a reason for hiding this comment

Uh oh!

weswigham commented Jul 11, 2016

Uh oh!

guncha commented Aug 8, 2016

Uh oh!

sandersn commented Sep 29, 2016

Uh oh!

sandersn commented Sep 29, 2016

Uh oh!

guncha commented Sep 30, 2016

Uh oh!

sandersn commented Sep 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sandersn commented Jul 8, 2016 •

edited

Loading