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

Remove webServer #51699

Merged
merged 5 commits into from
Dec 6, 2022
Merged

Remove webServer #51699

merged 5 commits into from
Dec 6, 2022

Conversation

sandersn
Copy link
Member

webServer is now moved to vscode-web, since that's the only project that was using it. This allows tsserver to be a node-only executable instead of conditionally being startable in a webworker.

Implementation notes

  1. I'm not at all sure I altered the generated namespace references correctly.
  2. I made tsserver unconditionally start now. It doesn't check for the existence of process as a proxy for "running on node". It just assumes it's running on node.

@jakebailey I would like for you to check those two things when you have a chance.

First draft; I may move some things around to be more readable.
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 30, 2022
1. Move StartSessionOptions to common next to where it's first used.
2. Inline single-use BaseLogger base class into its only child class,
Logger.
3. Start using direct imports, eg `import {} from './common'`. I hope
this is OK?!
@sandersn
Copy link
Member Author

sandersn commented Dec 1, 2022

@jakebailey The most recent commit removes { bundlerOptions: { exportIsTsObject: true } } from tsserver in herebyfile. I hope that's what you meant by making tsserver a normal cjs file again.

@orta
Copy link
Contributor

orta commented Dec 2, 2022

( Not that it's too important as the URLs are hidden, but it might be worth checking if you need to write an issue on vscode-web to remove the vscode-ts-playground code which relies on this (a search in the repo for 'playground' should be enough to know. )

@sandersn
Copy link
Member Author

sandersn commented Dec 2, 2022

@orta where is the vscode-ts-playground code? I searched in microsoft/vscode but didn't see anything that looked definitively like it.

@orta
Copy link
Contributor

orta commented Dec 2, 2022

Hrm, you might not have access then it's in the microsoft/vscode-web repo /cc @mjbvz

@sandersn
Copy link
Member Author

sandersn commented Dec 5, 2022

yep, microsoft/vscode-web is a 404 for me

@jakebailey
Copy link
Member

I would imagine that we'd want to switch that playground prototype into using tsserverlibrary as well, right?

@sandersn
Copy link
Member Author

sandersn commented Dec 5, 2022

yes, I just don't know where it is yet

@mjbvz
Copy link
Contributor

mjbvz commented Dec 6, 2022

I also don't have access to microsoft/vscode-web. Was this an old project that has since been deleted? Most of the vscode stuff for web should now be in the main vscode repo

@orta
Copy link
Contributor

orta commented Dec 6, 2022

Sorry, I mean the http server for vscode.dev - so that's probably the repo!

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

This looks good to me, though I believe that we may want to clean up the tsserver project a little now that there aren't two entrypoints. I had to do a bunch of movement in the module conversion to support the old web bundle, but that all is not needed now and we eliminate the common file and all of the exports and stuff.

@sandersn
Copy link
Member Author

sandersn commented Dec 6, 2022

@orta I looked for places in the vscode repo with mentions of tsserver and there are couple of mentions in comments in extensions/html-language-features, but that's it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants