Add server #2

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

Jump to bottom

Closed

GirlBossRush wants to merge 1 commit into 1.58.2-base from 1.58.2-server

Closed

Add server #2

GirlBossRush wants to merge 1 commit into 1.58.2-base from 1.58.2-server

Conversation

GirlBossRush

Copy link

@GirlBossRush GirlBossRush commented Aug 5, 2021

WIP

This PR cleans up much of our server protocol logic to use upstream’s existing IPC behaviors. Previously, we would translate WebSockets into a child process socket which behaves more consistently with the Electron approach to server communication. However, upstream has since made a more generic protocol interface which connect directly with a WebSocket.

Development usage

Open 4 terminals and run the following commands:

# Client watcher
yarn watch-client

# Web extensions watcher
yarn watch-web

# Start front-end server
yarn web

# Start back-end server
node out/cli.js --server localhost:8082 some_file_path

TODO

Document upstream’s usage of VSBuffer and how we parse message payloads
Move new interfaces to their respective files
Clean up debug code
Update our files to match upstream’s linter
Touch up CLI experience

@GirlBossRush GirlBossRush self-assigned this

Aug 5, 2021

@GirlBossRush GirlBossRush added the enhancement label

Aug 5, 2021

@GirlBossRush GirlBossRush requested review from jsjoeio, code-asher and oxy

August 5, 2021 14:34

@GirlBossRush GirlBossRush mentioned this pull request

Aug 5, 2021

Move vendored VScode to separate fork coder/code-server#3835

Closed

4 tasks

@jsjoeio

Copy link

jsjoeio commented Aug 5, 2021

This is a huge PR but I will review soon!

jsjoeio

jsjoeio reviewed

Aug 6, 2021

View reviewed changes

Copy link

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Great work @TeffenEllis!

I think the hardest part is knowing what you added compared to what was already there. Feel free to dismiss most things that are not related to things you changed.

Most comments are questions or nits.

I'm not going to approve since I think @code-asher should really be the one to approve since he has the most context on everything VSCode-related in code-server.

Overall though, super thankful you're tackling this and can't wait to be using this instead of the subtree we currently have 🙏

.eslintignore Outdated

**/extensions/markdown-language-features/notebook-out/**

**/extensions/typescript-basics/test/colorize-fixtures/**

**/extensions/**/dist/**

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

❓ Intentional?

.eslintrc.json

"target": "**/vs/server/**",

"restrictions": [

"vs/nls",

"**/vs/code/**/{common,server,browser,node,electron-sandbox,electron-browser}/**",

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

❓ Why do we add these two entries in here?

.vscode/settings.json

Comment on lines +87 to +92

"typescript.format.insertSpaceAfterConstructor": false,

"javascript.format.insertSpaceAfterConstructor": false,

"javascript.format.insertSpaceBeforeFunctionParenthesis": false,

"typescript.format.insertSpaceBeforeFunctionParenthesis": false,

"typescript.format.insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets": false,

"javascript.format.insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets": false,

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

❓Guessing you didn't add this, but do we know why we do this?

build/gulpfile.reh.js Outdated

Comment on lines 45 to 46

// NOTE@coder: Fix version due to .yarnrc removal.

return process.versions.node;

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

❓This seems like a temp fix. Should we open an issue to fix later?

build/lib/extensions.ts

input = updateExtensionPackageJSON(input, (data: any) => {

delete data.scripts;

deletedata.dependencies;

// https://github.com/cdr/code-server/pull/2041#issuecomment-685910322

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

👏

src/vs/server/protocol.ts Outdated

Comment on lines 30 to 43

// export const createSocketWrapper = (netSocket: net.Socket, { skipWebSocketFrames, permessageDeflate, inflateBytes }: ServerProtocolOptions): ISocket => {

// const nodeSocket = new NodeSocket(netSocket);

// if (skipWebSocketFrames) {

// return nodeSocket;

// }

// return new WebSocketNodeSocket(

// nodeSocket,

// permessageDeflate || false,

// inflateBytes || null,

// // Always record inflate bytes if using permessage-deflate.

// permessageDeflate || false,

// );

// };

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

remove?

src/vs/server/protocol.ts Outdated

Comment on lines 106 to 105

// Kick off the handshake in case we missed the client's opening shot.

// TODO: Investigate why that message seems to get lost.

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

Add follow-up issue?

src/vs/workbench/contrib/extensions/browser/extensionsViewlet.ts

Comment on lines +547 to +555

$('p', { style: 'margin-bottom: 0; display: flex; align-items: center' },

$('span', { class: 'codicon codicon-warning', style: 'margin-right: 2px; color: #C4A103' }),

'WARNING'),

$('p', { style: 'margin-top: 0; margin-bottom: 4px' },

'These extensions are not official. Find additional open-source extensions ',

$('a', { style: `color: ${linkColor}`, href: 'https://open-vsx.org/', target: '_blank' }, 'here'),

'. See ',

$('a', { style: `color: ${linkColor}`, href: 'https://github.com/cdr/code-server/blob/master/docs/FAQ.md#differences-compared-to-vs-code', target: '_blank' }, 'docs'),

'.'

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

this looks super janky but is that because it's jQuery? 😂

src/vs/base/common/ipc.d.ts Outdated

Comment on lines 57 to 73

// export interface Args {

// 'user-data-dir'?: string

// 'enable-proposed-api'?: string[]

// 'extensions-dir'?: string

// 'builtin-extensions-dir'?: string

// 'extra-extensions-dir'?: string[]

// 'extra-builtin-extensions-dir'?: string[]

// 'ignore-last-opened'?: boolean

// locale?: string

// log?: string

// verbose?: boolean

// _: string[]

// }

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

remove?

src/vs/base/common/ipc.d.ts Outdated

export interface OptionsMessage {

id: string

type: 'options'

// options: WorkbenchOptions

Copy link

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

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

remove?

jsjoeio

jsjoeio reviewed

Aug 9, 2021

View reviewed changes

Copy link

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Nice work fixing the .yarnrc notes!

Looks really good - I'm getting excited!

src/vs/server/entry.ts Outdated

Copy link

@jsjoeio jsjoeio Aug 9, 2021

Choose a reason for hiding this comment

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

Not sure if this is our todo or theres? (entry is our file right?)

Suggested change

// TODO use `socket.remoteAddress`

// TODO@coder use `socket.remoteAddress`

jsjoeio

jsjoeio reviewed

Aug 10, 2021

View reviewed changes

src/vs/server/server.ts Outdated

Comment on lines 69 to 70

Copy link

@jsjoeio jsjoeio Aug 10, 2021

Choose a reason for hiding this comment

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

Should we remove?

@GirlBossRush GirlBossRush force-pushed the 1.58.2-server branch from dd3adc1 to a5c23aa Compare

August 12, 2021 17:05

@GirlBossRush GirlBossRush force-pushed the 1.58.2-server branch from 558335d to 61f19b7 Compare

August 19, 2021 21:30

@GirlBossRush


 Flesh out lib/vscode migration. WIP

572311e

Fix issues surrounding IPC interfaces. Update names.
Fix spacing. Add missing property.
Flesh out web socket cleanup.
Flesh out web socket migration. WIP.
Fix issues surrounding web socket frames.
Touch up.
Combine web command with server. Allow JS interop.
Fix issues surrounding built in extensions.
WIP Fix issues surrounding net socket lifecycle, and frequent restarts.
Clean up socket lifecycle. Remove unused socket wrappers.
Fix issues where logger is defined before ready. Clean up extension lifecycle.

@GirlBossRush GirlBossRush force-pushed the 1.58.2-server branch from 61f19b7 to 572311e Compare

August 19, 2021 21:36

@GirlBossRush GirlBossRush mentioned this pull request

Aug 21, 2021

Clean up and refactor VSCode integration coder/code-server#4010

Merged

@GirlBossRush

Copy link

Author

GirlBossRush commented Oct 1, 2021

Resolved in coder/code-server#4010

@GirlBossRush GirlBossRush closed this

Oct 1, 2021

@GirlBossRush GirlBossRush deleted the 1.58.2-server branch

October 1, 2021 15:18

GirlBossRush pushed a commit that referenced this pull request

Nov 19, 2021

@Pranomvignesh


 Merge pull request #2 from microsoft/main

093353c

Updating Fork

Labels

enhancement

2 participants

@GirlBossRush @jsjoeio

Add server #2

Add server #2

Uh oh!

Conversation

@GirlBossRush GirlBossRush commented Aug 5, 2021

Development usage

TODO

Uh oh!

jsjoeio commented Aug 5, 2021

Uh oh!

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

GirlBossRush commented Oct 1, 2021

Uh oh!

Uh oh!