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

Refactor component analysis #504

Merged
merged 10 commits into from
Nov 6, 2017
Merged

Conversation

HerringtonDarkholme
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme commented Oct 27, 2017

Fix #502
This pull request also skips wrapping ts script object, so vetur and ts-loader will align.

The component analysis combines both AST traversal and type checking. More specifically, we only do type analysis on the definition object literal, e.g the argument of Vue.extend, which is found by AST traversal.

This approach is independent of Vue's type declaration. So both Vue2.4- and 2.5+ are supported.

export function createUpdater() {
const clssf = ts.createLanguageServiceSourceFile;
const ulssf = ts.updateLanguageServiceSourceFile;
return {
createLanguageServiceSourceFile(fileName: string, scriptSnapshot: ts.IScriptSnapshot, scriptTarget: ts.ScriptTarget, version: string, setNodeParents: boolean, scriptKind?: ts.ScriptKind): ts.SourceFile {
const sourceFile = clssf(fileName, scriptSnapshot, scriptTarget, version, setNodeParents, scriptKind);
if (isVue(fileName)) {
// store scriptKind info on sourceFile
Copy link
Member Author

@HerringtonDarkholme HerringtonDarkholme Oct 27, 2017

Choose a reason for hiding this comment

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

This might be the best workaround.We must to use one single languageService for both TS and JS. We cannot separate two language services to differentiate behavior. Otherwise two file types cannot interoperate with each other.

Also, createLanguageServiceSourceFile and its update version are singletons under ts namespace. We need to make these two function stateless. So recording scriptKind on sourceFile might be the only solution.

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise two file types cannot interoperate with each other.

Do we have to do that? It seems people are either using JS or TS (correct me if I'm wrong).

Also, I think we should start having a mapping from filePath -> template / style / script info, for things like #145 and #130.

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 believe people will do that, say, migration. Even if we can separate those two, createLanguageServiceSourceFile is a singleton. So we cannot change the behavior according to one language service, due to that multiple services will exist.

@HerringtonDarkholme HerringtonDarkholme changed the title [WIP] Disable wrapping TS script Refactor component analysis Oct 29, 2017
@HerringtonDarkholme
Copy link
Member Author

Ping @octref

This is ready to be reviewed.

@HerringtonDarkholme
Copy link
Member Author

Ping @octref

@octref
Copy link
Member

octref commented Nov 2, 2017

Sorry had been busy. I'll take a look tomorrow.

@octref octref self-assigned this Nov 2, 2017
@octref
Copy link
Member

octref commented Nov 6, 2017

Thanks, have been busy over other stuff.

@octref octref merged commit 7801a24 into vuejs:master Nov 6, 2017
@octref octref mentioned this pull request Nov 6, 2017
@octref
Copy link
Member

octref commented Nov 6, 2017

0.11 is not picking up this.$store and this.$router types.

Wondering if that's caused by this PR. I'll take a look later today too.

@HerringtonDarkholme
Copy link
Member Author

HerringtonDarkholme commented Nov 7, 2017

@octref It works as expected for me.

screen shot 2017-11-07 at 9 42 13 am

I guess the problem is from #515 . I tested vue-hackernews project which contains a tsconfig.json but doesn't specify allowJs: true. So #515 treat allowJs as false. And naturally TS doesn't import the typing file in Vuex because vue file doesn't import vuex.

After setting it $store is typed.

https://github.com/octref/veturpack/blob/master/tsconfig.json

@octref
Copy link
Member

octref commented Nov 7, 2017

I see, so veturpack's client/store/index.js never get read.
Having this as a default is confusing to beginners since they'd never figure it out. But I don't like the idea that Vetur sets allowJs to true when people have a js/ts config, since it would not be consistent with js/ts config which has allowJs default to false.

Think we should do a popup to suggest setting this, if they

  • have a tsconfig/jsconfig without allowJs
  • have a Vue file with JS section.

@HerringtonDarkholme
Copy link
Member Author

I don't think this would be that confusing. If the user uses jsconfig.json, allowJs is default to be true.

We can prioritize jsconfig.json if users primarily use JavaScript.

@octref
Copy link
Member

octref commented Nov 7, 2017

If the user uses jsconfig.json, allowJs is default to be true.

Oh that's a good point. I never checked that. That makes sense then. I'll add some docs to setup.

octref added a commit that referenced this pull request Nov 22, 2017
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.

2 participants