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

TS - mangle private and protected props #166126

Merged
merged 23 commits into from
Nov 16, 2022
Merged

TS - mangle private and protected props #166126

merged 23 commits into from
Nov 16, 2022

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Nov 11, 2022

This PR is an attempt (and work in progress) to minify/mangle our shipped bundled by a TypeScript to TypeScript transformation. In essence it detects and renames all private and protected members of all classes and renamed them to shorter names. My initial testing shows a reduction of ~1.8MB. This replaces an earlier effort that would do a JS to JS in-place transformation.

The status is that it successfully rewrites the VS Code sources base and that the rewritten code successfully compiles. There are a few test failures because of evil casting where a private property-access is enforced, like so or so.

This needs hardening and more testing but already looks promising. Random todos

  • get tests green
  • review and cleanup
  • put this into our build pipeline
  • think about generating source maps and check if TS ingests source maps?

@jrieken jrieken marked this pull request as draft November 11, 2022 16:36
@jrieken jrieken self-assigned this Nov 11, 2022
@jrieken jrieken added this to the November 2022 milestone Nov 11, 2022
@jrieken
Copy link
Member Author

jrieken commented Nov 11, 2022

FYI - This PR seems larger than it is because it changes the emit options for build/ to produce inline source maps

We have tests that use any-casts to make private properties accessible (in an untyped way). This fails when private property names are mangled and therefore they should be marked as public
@jrieken jrieken added the engineering VS Code - Build / issue tracking / etc. label Nov 14, 2022
@mjbvz
Copy link
Collaborator

mjbvz commented Nov 15, 2022

@jrieken Just pushed a change to try mangling webpacked extensions using a custom loader

@jrieken jrieken marked this pull request as ready for review November 16, 2022 12:47
build/lib/mangleTypeScript.ts Show resolved Hide resolved
@jrieken
Copy link
Member Author

jrieken commented Nov 16, 2022

@alexr00 Thanks for the review!

@ahinkle
Copy link
Contributor

ahinkle commented Dec 8, 2022

This PR broke popular (with 100K+ active installs) UI customization plugins. Would there be any room to support them with available methods, or would you consider adding more flexible options to customize the UI?

iocave/customize-ui#156
iocave/monkey-patch#55

@isidorn
Copy link
Contributor

isidorn commented Dec 8, 2022

@ahinkle thanks for reaching out. We already commented on the customize-ui extension here iocave/customize-ui#156 (comment)
More details can be found here microsoft/vscode-discussions#257

We would love to learn more about the missing customisations of the current UI, and for that it would be best to file a new issue, or upvote existing issues. After we get and process all the feedback we will decide which of the customisations we should explore to add. Thank you

@ahinkle
Copy link
Contributor

ahinkle commented Dec 8, 2022

Thanks @isidorn,

Users have been asking for years for customizable options in VSCode - it's no secret:

#519
#41909
#9285
#12377

A community extension was created to use the methods published by VSCode. It's unfortunate to see a breaking change to a largely used extension was pushed with minimal discussion in a 15-day timeframe with zero alternatives or fixes for users.

@kvndrsslr
Copy link

I second @ahinkle's opinion, it would have been good to initiate this dialogue first and after a solution is found, merge this.
Since the reasons communicated publicly for this are a mere 5% loading speed-up compared to un-mangled code it's even less understandable. I'm not sure anyone is actually hugely profiting from this - unless you guys actually want to kill monkey patching, in which case it should just be communicated like it is.

This software is a huge success and having developers as your user base can be overwhelming in terms of niche feature requests even for much smaller scale projects.
So I don't think Microsoft can, need or should tend to each of these UI customization requests individually. Just let developers do what they do best: hack systems to adjust them to their needs.

@stephenreid321
Copy link

stephenreid321 commented Dec 9, 2022

Outrageous, so many of us rely on customize-ui and monkey-patch, please find a way to allow these to continue working

@DeltaRazero
Copy link

Unfortunately, development time of features in VSCode slow down towards the end of the year. Of course, that is no problem since things like backlog cleanup of bugs and smaller issues is important too. However, given the average time needed to implement features has increased since VSCode has grown significantly in scope, it's annoying for users to give up on our customizations to improve UX and wait months — possibly years — before it gets implemented in the official VSCode repo.

@hammypants
Copy link

@ahinkle thanks for reaching out. We already commented on the customize-ui extension here iocave/customize-ui#156 (comment) More details can be found here microsoft/vscode-discussions#257

We would love to learn more about the missing customisations of the current UI, and for that it would be best to file a new issue, or upvote existing issues. After we get and process all the feedback we will decide which of the customisations we should explore to add. Thank you

You guys have literally closed issues on this in our face.

@chengJim
Copy link

It cause customize-ui unusable, so how can we customize the font family of the UI now ?

@binaryben
Copy link

Such a joke to break an extension that is providing customisation options that have literally been requested for years! I'm glad to hear that this is finally causing the PM to take note, but please revert this PR in the mean time until solutions have been provided. The absence of the customisations I have is significantly slowing down my workflow to the point I will have to downgrade and disable automatic updates.

@binaryben
Copy link

Extend, embrace, extinguish. So much for being for the open sourced community M$.

@panoply
Copy link

panoply commented Dec 11, 2022

Making my voice heard here. It's utterly wild and slightly baffling that folks needed to even rely on a monkey patch enhancements for the smallest tweaks like (for example) hiding the title bar. While this PR is great work overall, the fact that it has rendered the extension customize-ui inoperable is just ratchet.

While MS and the vscode team work tirelessly to deliver a great product the issue pertaining to the UI specifically the title bar for MacOS users is something that has persisted for several years. It's my hope that MS and the vscode team seriously consider exposing options for hiding the title bar and not continue to sit on the issue that quite literally has been the deciding factor of many when it come to choosing vscode over alternatives in the nexus. Personally, I didn't adopt vscode until I had the patch available.

Developers spend hours within the editors UI and our brains become accustomed. This is why folks have favourite themes, syntax colour quirks and font preferences. Developers coding in the confinements of laptop screens or those which don't leverage dual screens only have limited dimensions to work within and the title bar is a heathen which lives rent free and takes up space in an unapologetic manner.

v1.74.0 has disrupted the ui aesthetics for over 100k users who rely on customize-ui. Don't let vscode die on this hill.

@NuckChorris
Copy link

NuckChorris commented Dec 19, 2022

To everyone frustrated like me, you can download the previous version here and then set the following setting to prevent auto-updating:

"update.mode": "manual"

Sucks, but hopefully they revert it. I also recommend turning on Telemetry so they see us in their update adoption rate :)

@evolify

This comment was marked as abuse.

@neon-sunset
Copy link

neon-sunset commented Dec 22, 2022

This PR makes VS Code incompatible with its Customize UI and Monkey Patch extensions which were the only reasonable way to make the UI experience pleasant with proper layout and correct client-side decorations.

After this change has landed, me and a couple of my friends are considering dropping the usage of VS Code on a permanent basis and are already forced to rely on other tools.

@mylanconnolly
Copy link

I appreciate the desire to make the distributed application smaller, however it would be greatly appreciated if there was a mechanism to make things like Customize UI work in a similar fashion, again. This caught me by surprise and the default look of VSCode isn't very pleasant for me. Hopefully something will come along

@talks2much
Copy link

@mylanconnolly I think originally there was no intention to make application faster or smaller (<1.8MB is just not measurable), but to hide VS Code sources in bundle instead. It also broke a lot of other extensions on the marketplace that were using private properties (probably by mistake or lack of public API) and made extensions much harder to debug.

Never thought code obfuscation will start happening in OSS project.

@phen0menon

This comment was marked as abuse.

@rbreaves
Copy link

Locking my vscode version to 1.73 until this PR is either removed or made to work with Customize UI. Please remove this PR until/unless you collaborate with the Customize UI extension developer. @jrieken @alexr00 This addition makes VSCode unusable for me and however many tens of thousands.. maybe over 100k users that use Customize UI.

And yes, I agree with what @ahinkle says above.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
engineering VS Code - Build / issue tracking / etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.