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

Try to move import enhancement to server for better performance #181

Merged
merged 20 commits into from
Sep 6, 2020

Conversation

CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Sep 5, 2020

Try to move import enhancement(#172) to the server for better performance

SHOWCASE

import_enhancement

import_enhancement 2

import_enhancement 3

@CGQAQ CGQAQ marked this pull request as ready for review September 6, 2020 12:12
@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 6, 2020

@lucacasonato Please review this

@lucacasonato lucacasonato self-requested a review September 6, 2020 12:37
@lucacasonato
Copy link
Member

From my initial testing the std completion for import {} from "https://deno.land/", completes import {} from "https://deno.land/std/" instead of import {} from "https://deno.land/std@". Could you change this?

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 6, 2020

From my initial testing the std completion for import {} from "https://deno.land/", completes import {} from "https://deno.land/std/" instead of import {} from "https://deno.land/std@". Could you change this?

The std default to not need a version, should I just remove the trailing / ?
@ will trigger completion

@lucacasonato
Copy link
Member

I think it should default to using versioned imports, and if you give the @, people are more likely to actually set a version explicitly.

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 6, 2020

I think it should default to using versioned imports, and if you give the @, people are more likely to actually set a version explicitly.

I don't want to give the @ after the module name, because what if you already have a @ will result double @

And if you don't set fixed version, every time you trigger completion, it will send an HTTP GET request to fetch the latest version
I hope slow to complete the code will eventually force them to set a fixed version :)

import_enhancement 4

@lucacasonato
Copy link
Member

Thanks. @axetroy do you have any review comments?

core/permcache.ts Show resolved Hide resolved
@CGQAQ CGQAQ force-pushed the move_import_enhancement_to_server branch from 43861bd to 4497a18 Compare September 6, 2020 15:37
@CGQAQ
Copy link
Contributor Author

CGQAQ commented Sep 6, 2020

@axetroy Any others need to change?

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Thanks @CGQAQ for implementing this, and thanks @axetroy for the review

@lucacasonato lucacasonato merged commit d9f74ff into denoland:master Sep 6, 2020
@CGQAQ CGQAQ changed the title Try to move import enhancement to server for better performance Try to move import enhancement(#172) to server for better performance Sep 6, 2020
@CGQAQ CGQAQ changed the title Try to move import enhancement(#172) to server for better performance Try to move import enhancement to server for better performance Sep 6, 2020
CGQAQ added a commit to CGQAQ/vscode_deno that referenced this pull request Sep 13, 2020
@CGQAQ CGQAQ deleted the move_import_enhancement_to_server branch September 21, 2020 02:12
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