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 loading custom typescript.tsdk settings on web hosts #138211

Closed
wants to merge 4 commits into from

Conversation

weswigham
Copy link
Member

This allow you to set, eg,

{
    "typescript.tsdk": "https://unpkg.com/typescript@4.5.2/lib"
}

in vscode.dev and get a functioning custom tsdk version (for any version of TS published after microsoft/TypeScript#39656 was pulled in) - very useful for debugging custom versions and PRs in vscode.dev.

cc @orta @mattbierner

return [
new TypeScriptVersion(source,
serverPath.toString(),
/*DiskTypeScriptVersionProvider.getApiVersion(serverPath)*/ API.fromVersionString('4.4.1'), // TODO: Pull version form URL if possible
Copy link
Member Author

@weswigham weswigham Nov 30, 2021

Choose a reason for hiding this comment

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

Right now this API version is hardcoded - we could try and pull it from the URL (which could actually be anything, even a blob, so looking for a x.y.z in the url would be heuristic at best) or prefetch the document and search it for .versionMajorMinor = and pull out the version that way or assume the URL is a full directory structure and walk up to where we'd expect the package.json to be and fetch that. Unsure what we want to do - in either case we'd probably need to asyncify a few getters.

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 have a version that doesn't break any of the internal APIs by using a sync XMLHttpRequest to inspect the server's text contents for the version string. Could be OK - definitely would want to know if that or async'ing all these getters would be preferable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How much work would it be to use an async call? I worry that the sync request will stall all other extension operations

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative: could it say "Downloading..." and then show the ts sdk version from the API when it's finished loading and easy to access?

Copy link
Member Author

Choose a reason for hiding this comment

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

How much work would it be to use an async call? I worry that the sync request will stall all other extension operations

All the getters for different versions need to become getters for promises, and all of the functions using those getters need to become async. So pretty much everything version selector related. So long as that restructuring sounds fine, it's doable - just a large structural change for an otherwise pretty simple change. I'll finish up my local version of that for comparison - I originally bailed on it and prepped the sync request one (which shouldn't be that much worse than the sync FS calls in the non browser version) once I got to needing to refactor the getters types and thus existing APIs.

Copy link
Member Author

@weswigham weswigham Dec 7, 2021

Choose a reason for hiding this comment

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

So, I have an implementation with asyncness and lemme tell you: async is toxic. It had to flow out to everywhere.

Verified

This commit was signed with the committer’s verified signature.
weswigham Wesley Wigham

Verified

This commit was signed with the committer’s verified signature.
weswigham Wesley Wigham
return [
new TypeScriptVersion(source,
serverPath.toString(),
/*DiskTypeScriptVersionProvider.getApiVersion(serverPath)*/ API.fromVersionString('4.4.1'), // TODO: Pull version form URL if possible
Copy link
Collaborator

Choose a reason for hiding this comment

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

How much work would it be to use an async call? I worry that the sync request will stall all other extension operations

Verified

This commit was signed with the committer’s verified signature.
weswigham Wesley Wigham

Verified

This commit was signed with the committer’s verified signature.
weswigham Wesley Wigham
@orta
Copy link
Contributor

orta commented Dec 10, 2021

A solution for the display name: We thought that the URL passed could contain a query string which includes the user-presentable display name. E.g. instead of

{
    "typescript.tsdk": "https://unpkg.com/typescript@4.5.2/lib"
}

It could be:

{
    "typescript.tsdk": "https://unpkg.com/typescript@4.5.2/lib?display=4.5.2"
}

Any CDN will ignore the query params, and it can be safely given with the source

@weswigham
Copy link
Member Author

Technically it's more than just the display name - it also governs what features are enabled in the LS. It being in the user provided string as an override is pretty much the same as the user supplying it via the api version override option.

@mjbvz mjbvz modified the milestones: January 2022, February 2022 Jan 28, 2022
@Tyriar Tyriar modified the milestones: February 2022, March 2022 Feb 28, 2022
@mjbvz mjbvz modified the milestones: March 2022, April 2022 Mar 23, 2022
@mjbvz mjbvz modified the milestones: April 2022, On Deck Apr 20, 2022
@mjbvz
Copy link
Collaborator

mjbvz commented Oct 6, 2022

Closing as out of date but let me know if this is something we want to revisit

@mjbvz mjbvz closed this Oct 6, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants