GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add support to report errors in js files by mhegazy · Pull Request #14496 · microsoft/TypeScript

Conversation

Mohamed Hegazy (mhegazy) commented Mar 6, 2017 •

edited by DanielRosenwasser

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Adds ~~--checkJsFiles~~ --checkJs flag to enable error reporting in all .js files in a project (used in conjunction with --allowJs).
Adds a per-file version of the flag /// @check to enable the error reporting per file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Jessidhia, geta6, hanakla, HerringtonDarkholme, abenhamdine, niieani, christianbundy, SteveALee, markusjohnsson, ulrichb, and 12 more reacted with hooray emoji

All reactions

🎉 22 reactions

Mohamed Hegazy (mhegazy) added 4 commits

February 23, 2017 15:58


          Add support for handeling .js file correctelly in fixAddMissingMember…

91571f0

… code fix


          Add --checkJsFiles

0b1fff7


          Add support for //@check directives

9f0c5ce


          Add tests

0b247b1

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Mar 6, 2017

Jessica Franco (Jessidhia) commented Mar 7, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

This means that it'll be possible to opt-in to have types that can be inferred from the environment (lib, .ts, .d.ts or other parsed files) be checked, but everything else will be left as any, with closure compiler annotations (@type, @param, etc) for assertions?

Sounds pretty great for progressively migrating a codebase to typescript 🎉

I wonder how this interacts with noImplicitAny, though, since it's very hard to avoid in JS due to not being able to specify generic arguments, for example.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mohamed Hegazy (mhegazy) commented Mar 7, 2017

Copy link

Copy Markdown

Contributor Author

This means that it'll be possible to opt-in to check types that can be inferred from the environment (lib, .ts, .d.ts or other parsed files) will be checked, but everything else will be left as any, with closure compiler annotations (Type (@type), PARAM, etc) for assertions?

correct. this is the intention.

I wonder how this interacts with noImplicitAny, though, since it's very hard to avoid in JS due to not being able to specify generic arguments, for example.

I do not know what is the best solution here. We were discussing this topic today, and we are doing some additional inferences/modifications to types in a .js file that we do not do in a .ts file. some of these involve anys. not sure what --noImplicitAny means in this context though..

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Bill Ticehurst (billti) reviewed

Mar 7, 2017

View reviewed changes

Comment thread

tests/cases/compiler/checkJsFiles.ts Outdated

		@@ -0,0 +1,6 @@
		// @allowJs: true
		// @checkJsFiles: true

Bill Ticehurst (billti) Mar 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You might want to add @noEmit: true or @outFile to suppress all the Cannot write file... because it would overwrite errors in the baselines.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) added 2 commits

March 6, 2017 21:01


          Add --noEmit to tests

1f9bb69


          Allow @check directives to switch on/off checking in a file

b015c1d

Anders Hejlsberg (ahejlsberg) requested changes

Mar 7, 2017

View reviewed changes

Comment thread

src/compiler/commandLineParser.ts Outdated

		}
		},
		{
		name: "checkJsFiles",

Anders Hejlsberg (ahejlsberg) Mar 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Since we have allowJs, shouldn't this be checkJs? Having 'Files' on just one of them seems inconsistent.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) added 2 commits

March 7, 2017 13:48


          Change flag name to checkJs

9305d4d


          Error if --checkJs is used without --allowJs

fb218b7

Anders Hejlsberg (ahejlsberg) reviewed

Mar 8, 2017

View reviewed changes

Comment thread

src/compiler/program.ts Outdated

		// Instead, we'll report errors for using TypeScript-only constructs from within a
		// JavaScript file when we get syntactic diagnostics for the file.
		const checkDiagnostics = isSourceFileJavaScript(sourceFile) ? [] : typeChecker.getDiagnostics(sourceFile, cancellationToken);
		// For JavaScript files, we don't want to report semantic errors unless ecplicitlly requested.

Anders Hejlsberg (ahejlsberg) Mar 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Spelling

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/compiler/parser.ts Outdated

		}
		}

		const checkJsDirectiveRegEx = /^\/\/\/?\s*@check(\s+(true\|false))?/gim;

Anders Hejlsberg (ahejlsberg) Mar 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Or maybe an nocheck directive?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) added 12 commits

March 8, 2017 21:50


          Code review comments

e9f8214


          add es6 to buildProtocol

a202fa4


          Merge branch 'fixBuildBreak' into checkJSFiles

3d03f8d


          Disable check diagnostics per line

fe7719f


          Add quick fix to disable error checking in a .js file

706acdf


          Fix building webTestServer

13e80b9


          Add comment

936a91d


          Merge remote-tracking branch 'origin/updateCodeFixForAddMissingMember…

cc6affa

…' into checkJSFiles_QuickFixes


          Add debugging utilities

6e86596


          Support static properties

fd9fb8f


          Add disableJsDiagnostics codefixes to harnes

509b2dc


          Merge pull request #14568 from Microsoft/checkJSFiles_QuickFixes

1fbbead

Allow skipping diagnostics in .js file using comments and quick fixes to add them

This was referenced Mar 14, 2017

Allow skipping diagnostics in .js file using comments and quick fixes to add them #14568

Merged

Add support for handling .js file incorrectly in fixAddMissingMember code fix #14276

Merged

Mohamed Hegazy (mhegazy) added 2 commits

March 15, 2017 12:27


          Code review comments


          Merge branch 'master' into checkJSFiles

0dac29f

Vladimir Matveev (vladima) reviewed

Mar 15, 2017

View reviewed changes

Comment thread

src/compiler/program.ts

		function shouldReportDiagnostic(diagnostic: Diagnostic) {
		const { file, start } = diagnostic;
		const lineStarts = getLineStarts(file);
		let { line } = computeLineAndCharacterOfPosition(lineStarts, start);

Vladimir Matveev (vladima) Mar 15, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

can line have more than one error where only one should be suppresed
seems that ts-suppress should be located immediately before the line to be suppressed so if user will put a comment between ts-suppress and target line it will stop working. Is it intended?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Vladimir Matveev (vladima) reviewed

Mar 15, 2017

View reviewed changes

Comment thread

src/services/codefixes/disableJsDiagnostics.ts Outdated

		.map(d => allDiagnostcs[d].code);
		}

		function shouldCheckJsFile(sourceFile: SourceFile, compilerOptions: CompilerOptions) {

Vladimir Matveev (vladima) Mar 15, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this is the same snipped with this one. Can we share this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Refactor checking for checkJs value in a common helper

3b57b5d

Mohamed Hegazy (mhegazy) mentioned this pull request

Mar 17, 2017

Handling input file extensions other than .ts, .js, .tsx, and .jsx #10939

Closed

Mohamed Hegazy (mhegazy) added 2 commits

March 22, 2017 15:45


          Merge branch 'master' into checkJSFiles

e408cad


          Change ingore diagonstic comment to // @ts-ignore

db6c969

Ron Buckton (rbuckton) approved these changes

Mar 23, 2017

View reviewed changes

Comment thread

src/compiler/parser.ts

		}
		}

		const checkJsDirectiveRegEx = /^\/\/\/?\s(@ts-check\|@ts-nocheck)\s$/gim;

Ron Buckton (rbuckton) Mar 23, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would recommend you pull this out of the function and remove the g option. g is only really useful if you want to preserve lastIndex when using a regular expression in a loop. Also I would remove the m option as it should be unnecessary.

Consider this instead:

const checkJsDirectiveRegEx = /^\/\/\/?\s*@ts-(no)?check\s*$/i;
...
const checkJsDirectiveMatchResult = checkJsDirectiveRegEx.exec(comment);
if (checkJsDirectiveMatchResult) {
  checkJsDirective = {
    enabled: !!checkJsDirectiveMatchResult[1],
    end: range.end,
    pos: range.pos
  };
}

Capturing only the no allows you to avoid the additional case-insensitive string comparison.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) added 4 commits

March 27, 2017 10:46


          Merge branch 'master' into checkJSFiles

3378f5c


          Report semantic errors for JS files if checkJs is enabled

e630ab1


          Merge remote-tracking branch 'origin/master' into checkJSFiles

0637f24


          Merge remote-tracking branch 'origin/master' into checkJSFiles

8ea9617

Mohamed Hegazy (mhegazy) merged commit 0fd0903 into master

Mar 28, 2017

Basarat Ali Syed (basarat) mentioned this pull request

Apr 12, 2017

CheckJs option basarat/typescript-book#266

Open

Homa Wong (unional) commented Apr 22, 2017

Copy link

Copy Markdown

Contributor

Does this close #7661 ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mohamed Hegazy (mhegazy) commented Apr 22, 2017

Copy link

Copy Markdown

Contributor Author

Nope.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mohamed Hegazy (mhegazy) deleted the checkJSFiles branch

April 22, 2017 19:36

ifle (ifle) commented Apr 28, 2017

Copy link

Copy Markdown

Very nice option. But it will be more powerful if allows to suppression several errors

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Microsoft (microsoft) locked and limited conversation to collaborators

Jun 19, 2018

Reviewers

Anders Hejlsberg (ahejlsberg)

ahejlsberg requested changes

+3 more reviewers

Bill Ticehurst (billti)

billti left review comments

Vladimir Matveev (vladima)

vladima left review comments

Ron Buckton (rbuckton)

rbuckton approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Mohamed Hegazy (mhegazy) commented Mar 6, 2017 • edited by DanielRosenwasser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jessica Franco (Jessidhia) commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mohamed Hegazy (mhegazy) commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Homa Wong (unional) commented Apr 22, 2017

Uh oh!

Mohamed Hegazy (mhegazy) commented Apr 22, 2017

Uh oh!

ifle (ifle) commented Apr 28, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Mohamed Hegazy (mhegazy) commented Mar 6, 2017 •

edited by DanielRosenwasser

Loading

Uh oh!

There was an error while loading. Please reload this page.

Jessica Franco (Jessidhia) commented Mar 7, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.