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

Fix TS2318 and ignore .vscode-test folder #400

Merged
merged 1 commit into from
Jul 7, 2016

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Jul 7, 2016

Yay! We love PRs! 🎊

Please include a description of your change & check your PR against this list, thanks:

  • Commit message has a short title & issue references
  • Commits are squashed
  • It builds and tests pass (e.g gulp tslint)

Firstly ignore .vscode-test folder, otherwise it will slow down the auto-compilation. Secondly as we set noLib as true and we are using ES6 most of the time (currently we don't have any ES7 syntax yet), so I add es6 lib file to it.

@johnfn
Copy link
Member

johnfn commented Jul 7, 2016

👍 , though you can actually just delete vscode-test entirely and test within VSCode itself.

@johnfn johnfn merged commit 2b480c1 into VSCodeVim:master Jul 7, 2016
"node_modules",
".vscode-test"
],
"files": [
Copy link
Member

@jpoon jpoon Jul 7, 2016

Choose a reason for hiding this comment

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

I was looking into this a bit earlier, doesn't the files config define what files tsc will compile? (ref: https://www.typescriptlang.org/docs/handbook/tsconfig-json.html).

If no "files" property is present in a tsconfig.json, the compiler defaults to including all TypeScript (*.ts or *.tsx) files in the containing directory and subdirectories. When a "files" property is present, only the specified files are included.

Edit: Reading the documentation and SO posts, I'm so confused on what this attribute is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was referring to microsoft/TypeScript#3930 , seems if you set nolib as true you must add your own lib file as TypeScript compiler always rely on lib files to generate output. So the main purpose of nolib is allowing you to replace the original lib.d.ts to the one you want, here it's lib.es6.d.ts.

I'm not 100% sure whether it's correct but currently it's the only way to bypass the errors.

Copy link
Member

Choose a reason for hiding this comment

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

@jpoon you were correct; unfortunately, this change will break typescript compilation.

Be sure to test tsc --watch before making changes to tsconfig.json 😉

@rebornix
Copy link
Member Author

rebornix commented Jul 7, 2016

@johnfn the reason I always test those cases with command line is it's the same way as Travis does. Currently if you run tests inside VS Code, everything is good while if you run through command line, it will fail immediately. I mentioned this issue in Slack and it's due to HistoryTracker initialization.

@rebornix rebornix deleted the FixTSConfig branch July 7, 2016 05:01
@johnfn
Copy link
Member

johnfn commented Jul 7, 2016

The Travis thing is annoying, but it's not really indicative of how real people will use our extension. I put more weight on running the tests within VSCode because those results seem to correlate stronger with actual bugs existing in the code.

@rebornix
Copy link
Member Author

rebornix commented Jul 7, 2016

@jpoon @johnfn sorry for the break and thanks for @johnfn 's quick turnaround. The only thing that bothers me is, why we put noLib here. Per documentation, it's used for Do not include the default library file, then which file should it use? Currently we have no lib files in our code base, the result is when you run compile, you will always get TS2318 errors

> [email protected] compile /Users/rebornix/Documents/code/Vim
> node ./node_modules/vscode/bin/compile -watch -p ./

error TS2318: Cannot find global type 'Array'.
error TS2318: Cannot find global type 'Boolean'.
error TS2318: Cannot find global type 'Function'.
error TS2318: Cannot find global type 'IArguments'.
error TS2318: Cannot find global type 'Iterable'.
error TS2318: Cannot find global type 'IterableIterator'.
error TS2318: Cannot find global type 'Iterator'.
error TS2318: Cannot find global type 'Number'.
error TS2318: Cannot find global type 'Object'.
error TS2318: Cannot find global type 'RegExp'.
error TS2318: Cannot find global type 'String'.
error TS2318: Cannot find global type 'Symbol'.
error TS2318: Cannot find global type 'TemplateStringsArray'.
error TS2468: Cannot find global value 'Symbol'.
4:41:04 PM - Compilation complete. Watching for file changes.

I can ignore these things as it doesn't affect the functionality but the catch is, VS Code is relying on the lib file to do intellisense and since the lib file is missing, we are seeing hundreds of errors highlighting in editor, which makes the whole development hard.

Maybe I did something wrong with my local setup, any comments?

@johnfn
Copy link
Member

johnfn commented Jul 7, 2016

@rebornix My mistake. You're right about TS2318. We have some files in our typings/ directory that have been gitignore'd from an old version (including lib.es6). You don't have those files. So we don't see TS2318, but you do.

HOWEVER, we can't just add lib.es6 to the files array, because that unadds all of the files in src/, which breaks tsc --watch.

The correct solution for now is to just add back lib.es6 to the typings directory. When Typescript 2 rolls around, we can use glob stars in the files array, so we can do "src/**'

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants