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

Allow analyzing Vue files in node_modules #1979

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

tiagoroldao
Copy link
Contributor

  • Include ".vue" files in projectFiles (using ScriptKind.Deferred)
  • Use project files to determine virtual file interpolation (instead of checking for node_modules in path)

Fixes #1127

@octref octref force-pushed the tr/analyse-included-vue-modules branch from a66e469 to 2ecd808 Compare July 31, 2020 10:31
+ use project files to check for Virtual files
@octref octref force-pushed the tr/analyse-included-vue-modules branch from 2ecd808 to 56a2f0e Compare August 3, 2020 03:55
Copy link
Member

@octref octref left a comment

Choose a reason for hiding this comment

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

Good work. Thank you and sorry for my late response.

Is there a specific reason you are making the ScriptKind.Deferred change?
I tested and there I didn't see any problem, so I'm ok for merging it.

@octref octref merged commit 57b2141 into vuejs:master Aug 3, 2020
octref added a commit that referenced this pull request Aug 3, 2020
@tiagoroldao
Copy link
Contributor Author

Good work. Thank you and sorry for my late response.

Is there a specific reason you are making the ScriptKind.Deferred change?
I tested and there I didn't see any problem, so I'm ok for merging it.

@octref the config returned by getParsedConfig was not returning any vue filepaths, due to scriptKind not being set as Deferred (on commandLineParser.ts, which calls getSupportedExtensions)

This was masked by the fact that the extension checked for files itself (manually ignoring node_modules)

@tiagoroldao
Copy link
Contributor Author

@octref on a separate note, it may make sense to set this to an even more permissive:
PS an even more permissive setting would be:

export function isVirtualVueFile(path: string, projectFiles: Set<string>) {
  return path.endsWith('.vue.ts') && 
    (!path.includes('node_modules') || projectFiles.has(path.slice(0, -'.ts'.length)));

@octref
Copy link
Member

octref commented Aug 3, 2020

the config returned by getParsedConfig was not returning any vue filepaths, due to scriptKind not being set as Deferred (on commandLineParser.ts, which calls getSupportedExtensions)

@tiagoroldao Do you mind sending another PR that documents this inline?

PS an even more permissive setting would be:

👍 , would be great if you can combine the changes into one PR. Being a bit looser is better, as many users don't propoerly setup jsconfig/tsconfig, so some Vue files might not be included in the TS project.

@tiagoroldao
Copy link
Contributor Author

Will do.

octref added a commit that referenced this pull request Aug 4, 2020
@octref
Copy link
Member

octref commented Aug 4, 2020

@tiagoroldao I added your suggestion directly since I need to make a new release, and I want to get the isVirtualVueFile change in.

@tiagoroldao
Copy link
Contributor Author

Eheh, I was busy making the PR, only saw this now! Feel free to close #2112 if it is now irrelevant!

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.

Allow analyzing Vue files in node_modules
3 participants