chore: update Code to 1.68 #5263
Conversation
Was able to remove our changes to common/webview.ts since they are upstream now. Other than that no serious changes, just context diffs.
code-asher
force-pushed
the
code-update-1.68
branch
from
21d1a9e to
4e6e246
Compare
|
Thanks for doing this, I am eagerly awaiting 1.68 myself. I saw that this is marked as a draft, is there anything that you need help with? |
|
I think everything is good to go except the display language support. Upstream added their own display language support for web but it only supports certain built-in languages and it uses the browser default language. Ours has the user choose the display language via So the question is whether to:
Eventually upstream plans on supporting language extensions in web at which time we definitely would want to remove our patch but it would be unfortunate to lose that support at this stage as that would essentially be a regression so I am thinking we go with one of the first two options. I will be digging into it later today but if anyone has insight I am all ears! I am hoping it is straightforward though. |
|
Hmm yeah that's a tough problem indeed. It would suck to lose additional language support that we have added, but it also sucks to have to replace a core feature. I'm not sure what can be done for option 1, but if it's not feasible, I would prefer option 2 to losing languages in option 3. |
code-asher
force-pushed
the
code-update-1.68
branch
2 times, most recently
from
4070776 to
feb88ec
Compare
- Upstream moved the web socket endpoint so change the Express route from / to *. That will let web sockets work at any endpoint. - Everything in the workbench config is basically the same but de-indented (upstream extracted it into a separate object which resulted in a de-indent), the ordering is slightly different, and instead of vscodeBase we now need vscodeBase + this._staticRoute since everything is served from a sub-path now. - Move manifest link back to the root since that is where we host our manifest. - Change RemoteAuthoritiesImpl to use the same path building method as in other places (+ instead of using URI.parse/join). - Use existing host/port in RemoteAuthoritiesImpl and BrowserSocketFactory instead of patching them to use window.location (these are set from window.location to begin with so it should be the same result but with less patching). - Since BrowserSocketFactory includes a sub-path now (endpoints were changed upstream to serve from /quality/commit instead of from the root) the patch there has changed to prepend the base to that path (instead of using the base directly). - The workbench HTML now natively supports a base URL in the form of WORKBENCH_WEB_BASE_URL so no need for VS_BASE patches there anymore. - Upstream added type="image/x-icon" so I did as well. - Move the language patch to the end of the series so it is easier to eventually remove. - Remove the existing NLS config in favor of one that supports extensions. - Upstream deleted webview main.js and inlined it into the HTML so move that code (the parent origin check) into both those HTML files (index.html and index-no-csp.html). - The remaining diff is from changes to the surrounding context or a line was changed slightly by upstream (for example renamed files or new arguments like to the remote authority resolver).
code-asher
force-pushed
the
code-update-1.68
branch
from
feb88ec to
601d8d6
Compare
Codecov Report
@@ Coverage Diff @@
## main #5263 +/- ##
=======================================
Coverage 72.47% 72.47%
=======================================
Files 30 30
Lines 1671 1671
Branches 367 367
=======================================
Hits 1211 1211
Misses 397 397
Partials 63 63
Continue to review full report at Codecov.
|
|
I think this is good for testing and PR review. I went with option 2 for the display language so we have maintained the status quo. |
| @@ -1,17 +1,28 @@ | |||
| Add display language support | |||
|
|
|||
| This likely needs tweaking if we want to upstream. | |||
| We can remove this once upstream supports all language packs. | |||
There was a problem hiding this comment.
👏🏼 Thanks for updating this!
|
Ran into a couple things when testing the full build:
|
Code injects this into the client during the build process so it needs to be updated before we build.
code-asher
force-pushed
the
code-update-1.68
branch
from
b886ba8 to
a3dea91
Compare
|
@jsjoeio Three new commits fixing the aforementioned problems plus one spot where I could use the new base URL instead of the old one (both work for now but might as well use the new one). |
|
OK one more problem with our parent origin patch, on non-Firefox browsers the CSP breaks because there is a nonce so I need to update those. Give me a second... |
code-asher
force-pushed
the
code-update-1.68
branch
2 times, most recently
from
11087f9 to
50e4396
Compare
code-asher
force-pushed
the
code-update-1.68
branch
from
50e4396 to
ebe8451
Compare
Code overrides it with nothing. The date is also already injected.
code-asher
force-pushed
the
code-update-1.68
branch
3 times, most recently
from
a162dff to
e5f8d63
Compare
By just using the marketplace directly instead of going through the backend. I am not sure what the point is when searching extensions already goes directly to the marketplace anyway. But also remove the prefix that breaks this as well because otherwise existing installations will break.
code-asher
force-pushed
the
code-update-1.68
branch
from
e5f8d63 to
4355006
Compare
Closes #5165
I had a bit of trouble going straight to 1.68 (had some weird webview issues) so I went to 1.67 first then 1.68 and then it went smoothly. Not sure if that is a coincidence but seems like updating one version at a time might be the way to go.