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 vls initialize is slow #2458

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

yoyo930021
Copy link
Member

Fixed #2453

@yoyo930021 yoyo930021 force-pushed the fix-dependency-service-slow branch 2 times, most recently from 04a2cf2 to 280d7b5 Compare November 11, 2020 11:50
@yoyo930021 yoyo930021 force-pushed the fix-dependency-service-slow branch 2 times, most recently from 2d7928d to 4f023fc Compare November 11, 2020 12:14
@yoyo930021 yoyo930021 force-pushed the fix-dependency-service-slow branch 2 times, most recently from 20b9ed8 to d27bf3d Compare November 11, 2020 12:28
@rchl
Copy link
Collaborator

rchl commented Nov 11, 2020

I'm seeing multiple instances of [INFO ] Loaded typescript@4.0.5 from /Users/... during initialize.

It's not only loading typescript from the root of the node_modules but also within the dependencies if those include typescript.

I know that dependencies should generally not include typescript in dependencies but I have a couple of packages linked. But in any case, linked or not, should this be optimized to only try to load from the root of node_modules?

EDIT: Actually "only from root" would probably be wrong as packages can have some @namespace/* in the name. So then should vetur only look at dependencies that are explicitly included in dependencies and devDependencies?

@yoyo930021
Copy link
Member Author

I'm seeing multiple instances of [INFO ] Loaded typescript@4.0.5 from /Users/... during initialize.

It's not only loading typescript from the root of the node_modules but also within the dependencies if those include typescript.

I know that dependencies should generally not include typescript in dependencies but I have a couple of packages linked. But in any case, linked or not, should this be optimized to only try to load from the root of node_modules?

This code was prepared for monorepo.
People should be able to use different versions of TypeScript in their projects.
Although the TypeScript ecosystem doesn't seem to allow it.

@rchl
Copy link
Collaborator

rchl commented Nov 11, 2020

With the monorepo setup, the intention is that each sub-project will have its own package.json, right? So my suggestion should still be OK.

And it's not only about typescript. Other dependencies like prettier will also be searched and discovered in some deep sub-dependencies.

@yoyo930021
Copy link
Member Author

yoyo930021 commented Nov 11, 2020

With the monorepo setup, the intention is that each sub-project will have its own package.json, right? So my suggestion should still be OK.

And it's not only about typescript. Other dependencies like prettier will also be searched and discovered in some deep sub-dependencies.

Your suggestion is good.
But this logic was prepared for the other libraries that came before it.

const result = readPkgUp.sync({ cwd: fspath, normalize: false });

The main thing is that I want to be as consistent as possible with the require behavior.

@rchl
Copy link
Collaborator

rchl commented Nov 11, 2020

But require wouldn't find dependency c located at node_modules/a/node_modules/c unless it would be called from node_modules/a/index.js, for example. So I'm not sure how current behavior is equivalent to require.

Note that I might not be seeing the whole picture...

@yoyo930021
Copy link
Member Author

But require wouldn't find dependency c located at node_modules/a/node_modules/c unless it would be called from node_modules/a/index.js, for example. So I'm not sure how current behavior is equivalent to require.

Note that I might not be seeing the whole picture...

You're right.

@yoyo930021 yoyo930021 force-pushed the fix-dependency-service-slow branch from d27bf3d to e88c26b Compare November 11, 2020 13:07
@yoyo930021
Copy link
Member Author

yoyo930021 commented Nov 11, 2020

@rchl Thanks a lot.
Can you help me test your project for this branch?

@rchl
Copy link
Collaborator

rchl commented Nov 11, 2020

Looks good with the latest changes. It didn't find any extraneous typescript dependencies (only the single, expected one) or any other dependencies. And it was relatively fast (~2s for initialize response).

@yoyo930021 yoyo930021 merged commit 97b245a into vuejs:master Nov 11, 2020
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.

VLS initialize is slow
2 participants