Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

fix: add handle for resolveExternalUri #5624

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

Merged
jsjoeio merged 19 commits into main from jsjoeio/external-uri
Oct 14, 2022
Merged

fix: add handle for resolveExternalUri #5624

jsjoeio merged 19 commits into main from jsjoeio/external-uri
Oct 14, 2022

Conversation

Copy link
Contributor

@jsjoeio jsjoeio commented Oct 6, 2022 •
edited
Loading

This PR fixes asExternalUri to use code-server's built-in proxy or VSCODE_PROXY_URI if it's been set.

Todos

  • handle path-based proxies
  • add e2e test for it
  • add e2e test for VSCODE_PROXY_URI
  • support VSCODE_PROXY_URI

Fixes #1936

ksylvan reacted with heart emoji
@jsjoeio jsjoeio self-assigned this Oct 6, 2022
Copy link
Member

code-asher commented Oct 6, 2022 •
edited
Loading

Patch patches/proxy-uri.diff does not apply (enforce with -f)

Might be easiest to force-apply it then resolve the conflicts and refresh it! It could be a conflict with base-path.diff.
jsjoeio reacted with thumbs up emoji

Copy link
Contributor Author

jsjoeio commented Oct 6, 2022

Oops..looks like proxyEndpointTemplate adds /proxy/port for you.

image

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.

Awesome work!

@jsjoeio jsjoeio temporarily deployed to npm October 6, 2022 18:56 Inactive
Copy link

github-actions bot commented Oct 6, 2022 •
edited
Loading

✨ code-server dev build published to npm for PR #5624!

  • Last publish status: success
  • Commit: bfe91fc

To install in a local project, run:

npm install @coder/code-server-pr@5624

To install globally, run:

npm install -g @coder/code-server-pr@5624

Copy link

codecov bot commented Oct 6, 2022 •
edited
Loading

Codecov Report

Merging #5624 (bfe91fc) into main (714afe0) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph 

@@ Coverage Diff @@
## main #5624 +/- ##
=======================================
 Coverage 72.61% 72.61% 
=======================================
 Files 30 30 
 Lines 1680 1680 
 Branches 368 368 
=======================================
 Hits 1220 1220 
 Misses 397 397 
 Partials 63 63

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 714afe0...bfe91fc. Read the comment docs.

Copy link
Contributor Author

jsjoeio commented Oct 6, 2022

Looks good now! (ignore expected, hard-coded)

image

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.

Looks great to me! Do you want an approval now to merge or are you going to add the domain-based proxy in this same PR so I should approve after that is done?

jsjoeio reacted with hooray emoji
@jsjoeio jsjoeio temporarily deployed to npm October 6, 2022 19:10 Inactive
Copy link
Contributor Author

jsjoeio commented Oct 6, 2022

are you going to add the domain-based proxy in this same PR so I should approve after that is done?

I'll work on that here then mark PR ready when all is said and done. Thanks for the early review!
code-asher reacted with rocket emoji

@jsjoeio jsjoeio temporarily deployed to npm October 7, 2022 20:26 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 7, 2022 21:58 Inactive
@jsjoeio jsjoeio marked this pull request as ready for review October 7, 2022 22:20
@jsjoeio jsjoeio requested a review from a team as a code owner October 7, 2022 22:20
@jsjoeio jsjoeio temporarily deployed to npm October 7, 2022 22:24 Inactive
Copy link
Contributor Author

jsjoeio commented Oct 10, 2022

It appears the display language e2e test is failing. I'm going to see if I can reproduce locally.

@jsjoeio jsjoeio temporarily deployed to npm October 10, 2022 21:08 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 10, 2022 21:21 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 10, 2022 21:25 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 10, 2022 21:49 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 10, 2022 22:07 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 10, 2022 22:21 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 11, 2022 01:39 Inactive
Comment on lines 32 to 37
"__metadata": {
"id": "47e020a1-33db-4cc0-a1b4-42f97781749a",
"publisherDisplayName": "MS-CEINTL",
"publisherId": "0b0882c3-aee3-4d7c-b5f9-872f9be0a115",
"isPreReleaseVersion": false
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed VS Code added this after running the test so I decided to commit it.

code-asher reacted with thumbs up emoji
this.codeServer.logger.debug("Waiting for test extension to load...")

await this.page.waitForSelector(selector)
await this.page.waitForSelector(selector,{timeout: 5000})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gonna drop this change

code-asher reacted with thumbs up emoji
// yarn test:e2e --workers 1 # Run with one worker
// yarn test:e2e --project Chromium # Only run on Chromium
// yarn test:e2e --grep login # Run tests matching "login"
// PWDEBUG=1 yarn test:e2e # Run Playwright inspector
Copy link
Contributor Author

Choose a reason for hiding this comment

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

saves time from having to look this up each time 😂

code-asher reacted with thumbs up emoji
Copy link
Member

@code-asher code-asher Oct 13, 2022

Choose a reason for hiding this comment

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

100% lol same reason I added the other comments I am always forgetting the flags

Copy link
Contributor Author

jsjoeio commented Oct 13, 2022

@code-asher ready for another review!

@jsjoeio jsjoeio temporarily deployed to npm October 13, 2022 18:06 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 13, 2022 20:18 Inactive
@jsjoeio jsjoeio temporarily deployed to npm October 13, 2022 20:30 Inactive
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.

Nicely done! I only have a comment about the added metadata.

@jsjoeio jsjoeio enabled auto-merge (squash) October 13, 2022 22:20
@jsjoeio jsjoeio temporarily deployed to npm October 13, 2022 22:22 Inactive
Copy link

ericzhucode commented Feb 15, 2023 •
edited
Loading

I found there's a conflict between this feature and url displayed in open external uri dialog.
If you open an external uri for the first time then the domain of it will be resolved to cache in map data structure.
Like https://www.w3schools.com/c/c_structs.php
image

For the next time if you open a different uri with the same domain
Like https://github.com/coder/code-server/issues
it will be resolved as the last uri which has been cached.
image

The uri displayed here is not correct

Before there's no resolvers registered by default so external uri is not resolved but after this feature added, cache will take effect
image

Hope there's a solutaion for this. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

asExternalUri doesn't work

AltStyle によって変換されたページ (->オリジナル) /