Vscode: Remove some deprecated code in ./src/vs/base

[LeuisKen]

I find that following methods are deprecated:

7 results - 2 files

src/vs/base/browser/dom.ts:
  74  };
  75  
  76: /** @deprecated ES6 - use classList*/
  77  export function hasClass(node: HTMLElement | SVGElement, className: string): boolean { return _classList.hasClass(node, className); }
  78: /** @deprecated ES6 - use classList*/
  79  export function addClass(node: HTMLElement | SVGElement, className: string): void { return _classList.addClass(node, className); }
  80: /** @deprecated ES6 - use classList*/
  81  export function addClasses(node: HTMLElement | SVGElement, ...classNames: string[]): void { return _classList.addClasses(node, ...classNames); }
  82: /** @deprecated ES6 - use classList*/
  83  export function removeClass(node: HTMLElement | SVGElement, className: string): void { return _classList.removeClass(node, className); }
  84: /** @deprecated ES6 - use classList*/
  85  export function removeClasses(node: HTMLElement | SVGElement, ...classNames: string[]): void { return _classList.removeClasses(node, ...classNames); }
  86: /** @deprecated ES6 - use classList*/
  87  export function toggleClass(node: HTMLElement | SVGElement, className: string, shouldHaveIt?: boolean): void { return _classList.toggleClass(node, className, shouldHaveIt); }
  88  

src/vs/base/common/strings.ts:
  15  
  16  /**
  17:  * @deprecated ES6: use `String.padStart`
  18   */
  19  export function pad(n: number, l: number, char: string = '0'): string {

I'd like to send a pull request to remove these deprecated methods and refactor all usages if you want.

[rebornix]

Starting from @mjbvz , not sure where the deprecation information are from.

[jrieken]

I'd like to send a pull request to remove these deprecated methods

:-1: We do not accept pull requests like that

[jrieken]

This is driven from .vscode/searches/es6.code-search and each and every user of the listed functions needs to become active.

[jrieken]

The classList usage is usually a big change, the others should be small... Starting with @joaomoreno, please pass it on to the next person

[joaomoreno]

@jrieken I am out this month... sorry.

[mjbvz]

Remaining:

src/vs/base/common/strings.ts

• [x] pad
• [x] startsWith
• [x] endsWith
• [x] repeat

src/vs/base/common/objects.ts

• [x]  assign

src/vs/base/browser/dom.ts

• [x] removeNode
• [x] hasClass and friends

src/vs/base/common/arrays.ts

• [x] first
• [x] find

[isidorn]

I have replaced my usages of the deprecated code. I believe I have cought all places, however if some remain feel free to ping me.
Assigning this to @joaomoreno since I saw a lot of usages in our widgets, though feel free to pass it on to the next person.

[joaomoreno]

Here's a useful find/replace input pair, for text based refactoring:

(add|remove|toggle)Class\(([^,]+), 
$2.classList.$1(

[joaomoreno]

The unfortunate choice of having multiple class names on a single string Codicon.classNames: string and having DOM.addClass support that is the hardest problem here. Going with simple refactoring, powered by TypeScript, you immediately hit very bad things:

[joaomoreno]

After looking at all references from here, I believe I got a good picture of the top 4 people who would do most of the rest of the changes: @sbatten @bpasero @jrieken @alexdima .

Again, be aware of codicons, which use multiple classes in a single string and classList.add will blow up in your face whenever it sees whitespace.

[bpasero]

@joaomoreno I missed it, what should I do if my class contains a space?

[isidorn]

Hmm I see that strings.pad is deprecated in favor of String.padStart. But they do not have the same functionality. Am I missing something here?
Code pointer https://github.com/microsoft/vscode/blob/fcb916f02c49a05a841663f319b8d9dbb3ee2daa/src/vs/base/common/strings.ts#L19

[jrieken]

Isn't strings.pad(1, 10, '0') === String(1).padStart(10, '0')

[isidorn]

@jrieken thanks. I have misread the method.

[sbatten]

I believe the change I just pushed removes the last usages of removeNode

[bpasero]

I don't think I own any of the remaining ones but I went through hasClass across all usages, I think it's better someone just does them all:

[jrieken]

@bpasero there was still some usage of the strings#repeat in your land

[bpasero]

@jrieken can you point me to that code please

[jrieken]

src/vs/platform/diagnostics/node/diagnosticsService.ts you were the last to touch it

[bpasero]

@RMacfarlane can you please see https://github.com/microsoft/vscode/commit/02710ecca2d675e98e411f07a2f8879ffc3aba41 if it works as you intended it, I think you are the original author.

[RMacfarlane]

Thanks @bpasero, your changes look good!

[jrieken]

almost there... @joaomoreno @alexdima @sandy081 @mjbvz for strings#pad

[joaomoreno]

I've deleted strings#pad.

[mjbvz]

Remaining (all under dom.ts):

• [ ] [addClass](https://github.com/microsoft/vscode/blob/cd2a92ae134d4eb20f20f8f6250a4a89c86e7abb/src/vs/base/browser/dom.ts#L72)
• [x] [addClasses](https://github.com/microsoft/vscode/blob/cd2a92ae134d4eb20f20f8f6250a4a89c86e7abb/src/vs/base/browser/dom.ts#L72)
• [x] [removeClasses](https://github.com/microsoft/vscode/blob/cd2a92ae134d4eb20f20f8f6250a4a89c86e7abb/src/vs/base/browser/dom.ts#L72)
• [ ] [toggleClass](https://github.com/microsoft/vscode/blob/cd2a92ae134d4eb20f20f8f6250a4a89c86e7abb/src/vs/base/browser/dom.ts#L72)

@joaomoreno Assigning you to start. Please reassign to correct owners for this

[joaomoreno]

• Terminal @Tyriar
• Webview @mjbvz
• Scrollbar @alexdima
• Lightbulb Widget @aeschli
• Workbench Theme Service @aeschli

[jrieken]

oh, wow actually done

[jrieken]

👏 and thanks everyone involved here