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.

Sign up for GitHub

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

Use upstream server #4414

Merged
merged 29 commits into from
Nov 10, 2021
Merged

Use upstream server #4414

merged 29 commits into from
Nov 10, 2021

Conversation

GirlBossRush
Copy link
Contributor

@GirlBossRush GirlBossRush commented Oct 29, 2021 •
edited by jsjoeio
Loading

This PR updates code-server to lean more on upstream’s server behaviors.

See code-server-v2 branch on our fork additional changes: https://github.com/cdr/vscode/compare/main...cdr:code-server-v2?expand=1

Breaking changes so far:

  • extra-extensions-dir flag removed
  • extra-builtin-extensions-dir flag removed
  • Static endpoint cannot reach outside code-server anymore (/vcode-remote-resource can though)
  • Experimental compression feature flag removed (is probably the default now) (not really breaking as this is not exposed)
  • install-source flag removed
  • locale flag removed
  • no "Logout" in menu
  • Help > About doesn't show code-server version
  • Installed Extensions doesn't show installed extensions
  • Vim extension does not work
  • Marketplace override format

TODO:

  • Patch product.json with our values, Open VSX
  • Locale flag needed?
  • Install source flag needed?
  • Add back nfpm fix

Moved to separate issue:

To test:

  • Chrome
  • Firefox
  • Safari
  • Password authentication
  • Search, edit, other basic functionality
  • Caching
  • Opening to a directory or workspace on the command line
  • Disconnections
  • Heartbeat
  • Display language (localization)
  • Opening a file in an existing instance
  • Log out
  • PWA
  • Some extensions (Java, Python, Jupyter, Vim, themes, icon themes)
    • icon themes
    • Python
    • Vim
    • Jupyter
    • Java
    • themes
  • Reverse proxy (NGINX, Caddy) (HTTP, HTTPS, base path, no base path, trailing slash, no trailing)
  • Debugging
  • Terminal, terminal persistence
  • Hot reload (pkill -SIGUSR1 -f code-server)
  • Running code-server in code-server
  • Log level gets set correctly in client
  • Update notification
  • /vscode endpoint (not working without trailing slash, otherwise it seems /anystring/ shows VS Code)
  • Plugins
  • GitHub auth
  • Diff menu Source Control context menu missing some items #1104
  • CODE_SERVER_* variable sanitization in terminal

@GirlBossRush GirlBossRush added enhancement Some improvement that isn't a feature high-priority This issue needs to be resolved ASAP labels Oct 29, 2021
@GirlBossRush GirlBossRush self-assigned this Oct 29, 2021
@GirlBossRush GirlBossRush requested a review from a team as a code owner October 29, 2021 19:56
@github-actions
Copy link

github-actions bot commented Oct 29, 2021 •
edited
Loading

✨ Coder.com for PR #4414 deployed! It will be updated on every commit.

jsjoeio
jsjoeio reviewed Oct 29, 2021
View reviewed changes
Copy link
Contributor

@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.

Most of this looks good to me 👍

Biggest question: what's the motivation for changing port from number -> string? And is it considered a breaking change?

ci/build/build-vscode.sh Outdated Show resolved Hide resolved
src/node/app.ts Outdated Show resolved Hide resolved
src/node/cli.ts Outdated Show resolved Hide resolved
src/node/cli.ts Show resolved Hide resolved
src/node/cli.ts Outdated Show resolved Hide resolved
src/node/main.ts Outdated Show resolved Hide resolved
@GirlBossRush GirlBossRush mentioned this pull request Oct 30, 2021
Merged
jsjoeio
jsjoeio previously requested changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@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.

No concerns from me! 👍 Requesting changes since there are conflicts (and I see some todos in the description still). Once those are sorted, lmk and I'll test locally for regressions!

src/node/cli.ts Show resolved Hide resolved
@jsjoeio jsjoeio mentioned this pull request Nov 1, 2021
Closed
code-asher
code-asher reviewed Nov 2, 2021
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have not been able to run this with watch or build yet but it looks very promising!! Wondering if this should be a v4.

ci/build/build-vscode.sh Outdated Show resolved Hide resolved
src/node/cli.ts Outdated Show resolved Hide resolved
src/node/cli.ts Show resolved Hide resolved
src/node/cli.ts Outdated Show resolved Hide resolved
src/node/cli.ts Show resolved Hide resolved
src/node/cli.ts Show resolved Hide resolved
src/node/routes/vscode.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio mentioned this pull request Nov 3, 2021
Closed
@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 3, 2021

Just fixed the conflicts. But there are ton of TypeScript/testing errors to fix now that port is gone. Marking this as draft and going to start working on them.

@jsjoeio jsjoeio self-assigned this Nov 3, 2021
@jsjoeio jsjoeio marked this pull request as draft November 3, 2021 18:14
@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 3, 2021

There is a lot to be fixed with this and we need to prioritize getting a release out ASAP so we're going to wait until @TeffenEllis is back to help with this.

GirlBossRush and others added 2 commits November 4, 2021 12:25
@GirlBossRush
bump vscode.
062ce32
@code-asher
Add platform to vscode-reh-web task
2481711 
Our strategy has been to build once and then recompile native modules
for individual platforms.  It looks like VS Code builds from scratch for
each platform.

But we can target any platform, grab the pre-packaged folder, then
continue with own packaging.

In the future we may want to rework to match upstream.
@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 9, 2021

Running node . <file> in the Integrated Terminal in code-server opens it in code-server (as expected)

image

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 9, 2021

update notification still works! 🎉

image

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 9, 2021

Need to look into e2e tests. I think we're getting a focus error with the Integrated Terminal test because something on the Getting Started page is taking focus which means we can't click the Command Palette in the menu.

image

@code-asher
Use user settings to disable welcome page
89cc5a9 
The workspace setting seems to be recognized but if so it is having no
effect.
@code-asher code-asher marked this pull request as ready for review November 10, 2021 04:36
@code-asher code-asher self-requested a review November 10, 2021 04:37
code-asher
code-asher approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

We will be knocking out the remaining tasks in separate PRs.

@code-asher code-asher dismissed jsjoeio’s stale review November 10, 2021 04:41

Comments have been addressed, in particular the port is a number again :)

@code-asher code-asher merged commit 1b60ef4 into main Nov 10, 2021
@code-asher code-asher deleted the upstream-server-fixes branch November 10, 2021 05:28
@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 10, 2021

I will open new issues and create a milestone for the separate tasks!

This was referenced Nov 10, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Nov 10, 2021
Closed
Closed
@jsjoeio jsjoeio mentioned this pull request Nov 23, 2021
Closed
@jsjoeio jsjoeio changed the title Use upstream server WIP Use upstream server Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsjoeio jsjoeio jsjoeio left review comments

@code-asher code-asher code-asher approved these changes

Assignees

@GirlBossRush GirlBossRush

@jsjoeio jsjoeio

@code-asher code-asher

Labels
enhancement Some improvement that isn't a feature high-priority This issue needs to be resolved ASAP
Projects
None yet
Milestone
4.0.0 - pre-release
Development

Successfully merging this pull request may close these issues.
3 participants
@GirlBossRush @jsjoeio @code-asher