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

Updates typescript to 2.7.1 version #686

Merged
merged 5 commits into from
Feb 10, 2018
Merged

Updates typescript to 2.7.1 version #686

merged 5 commits into from
Feb 10, 2018

Conversation

goszczynskip
Copy link
Contributor

In this TS version Definite assignment assertions were added, that's why this PR contains more than just package update.

@@ -294,7 +295,7 @@ export function getJavascriptMode(
const program = service.getProgram();
definitions.forEach(d => {
const sourceFile = program.getSourceFile(d.fileName);
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme Feb 9, 2018

Choose a reason for hiding this comment

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

definitive assertion should add here. Getting sourceFile from declaration is expected to be non null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I doesn't get that. getSourceFile returns ts.SourceFile | undefined in 2.7.1. To get rid of errors I had add to add exclamation mark on 298. It is not allowed to add it on line:col (296:25). Another solution is to add if with return statement.

Copy link
Member

Choose a reason for hiding this comment

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

public closed: boolean;
public endTagStart: number;
public attributes: { [name: string]: string };
public tag!: string;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since 2.7.1 it isn't allowed to declare properties without initialization when strict flag is set inside tsconfig. Typescript throws there error even though you null check that later.

Copy link
Member

Choose a reason for hiding this comment

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

You can declare it as optional or typing it as string | undefined

@@ -37,7 +37,7 @@ export function getTagProviderSettings(workspacePath: string | null | undefined)
return settings;
}
try {
const packagePath = ts.findConfigFile(workspacePath, ts.sys.fileExists, 'package.json');
const packagePath = ts.findConfigFile(workspacePath, ts.sys.fileExists, 'package.json') || '';
Copy link
Member

Choose a reason for hiding this comment

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

If package is not found, it should return early. Empty path doesn't make much sense.

@@ -27,7 +27,7 @@ export function testDSL(setup: CompletionTestSetup): (text: TemplateStringsArray
}

export class CompletionAsserter {
lastMatch: CompletionItem;
lastMatch!: CompletionItem;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like lastMatch is better modeled as CompletionItem | undefined. https://github.com/vuejs/vetur/pull/686/files#diff-8c1c99640c5e32e1403b16bc16ec3849R61

But I'm fine with current writing since lastMatch is always called after has.

@HerringtonDarkholme
Copy link
Member

I didn't write htmlParser myself, but it seems most parser's properties are indeed optional. I don't think bang them all is a good practice. @goszczynskip Would you please declare them as optional?

@goszczynskip
Copy link
Contributor Author

Sure, I'll do that but I can push changes this evening.

@octref
Copy link
Member

octref commented Feb 9, 2018

I don't think updating TS dependency like this is a long-term solution.

First, user still can't select TS versions. Second, TS updates also bring in a lot of updates in the LS API which powers all language features in the <script> section. We should test on those too.

But I'm not against upgrading to 2.7.1 or having strictPropertyInitialization. Just pointing out the side effect and that this would not solve #682 in the long term.

@goszczynskip
Copy link
Contributor Author

I can totally agree with that. I think one solution would be to select ts version on bottom bar, the same way you choose for .ts files. Right now I see this PR as quick update to let users benefit from eg. Definite assignment assertions

@HerringtonDarkholme
Copy link
Member

Thanks, upgrading looks good!

We can solve #682 later. Upgrading itself is good to merge.

@DanielRosenwasser
Copy link
Contributor

Any chance we can get a release with this out? I'd like to fix microsoft/TypeScript-Vue-Starter#36, but know users will report issues in Vetur.

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.

4 participants