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

chore(ts): small tweaks to make typescript happier #137

Closed
wants to merge 1 commit into from

Conversation

isaacs
Copy link

@isaacs isaacs commented Apr 28, 2023

  • Use trimStart/trimEnd instead of deprecated trimLeft/trimRight
  • Target es2019, which is when trimLeft/Right/Start/End methods became available (at least according to tsc)

@shadowspawn
Copy link
Member

These changes look reasonable, but under what circumstances does TypeScript complain?

(I am not seeing any messages from running tsc, or in Visual Studio.)

@isaacs
Copy link
Author

isaacs commented Apr 29, 2023

The ts server in neovim warned about them. Maybe I have a different typescript version or something? Or it could be using ts lint? I've found its advice is usually pretty good.

@shadowspawn
Copy link
Member

https://exploringjs.com/es2018-es2019/ch_array-prototype-trimstart-trimend.html

Many web browsers have the string methods .trimLeft() and .trimRight(). Those were added to Annex B of the ECMAScript specification (as aliases for .trimStart() and .trimEnd()): features that are required for web browsers and optional elsewhere.

https://tc39.es/ecma262/

ECMAScript 2019 introduced a few new built-in functions: ... trimStart and trimEnd on String.prototype as better-named alternatives to the widely implemented but non-standard String.prototype.trimLeft and trimRight built-ins.

Node.js has trimStart and trimEnd in v12: https://node.green

@shadowspawn
Copy link
Member

The Typescript configuration docs mention trimStart in the lib section (lib change is implied by changing target):
https://www.typescriptlang.org/tsconfig#lib

shadowspawn
shadowspawn previously approved these changes Apr 29, 2023
Copy link
Member

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

LGTM
The change to target makes this semver major, but I expect we'll be doing a release to drop node 12 (and maybe 14 too) at some point anyway.

@shadowspawn shadowspawn dismissed their stale review April 29, 2023 23:48

Still don't understand the warning situation

@shadowspawn
Copy link
Member

I finally managed to get some warnings from tsc compile by removing @types/node, which has:

// Node.js ESNEXT support
interface String {
    /** Removes whitespace from the left end of a string. */
    trimLeft(): string;
    /** Removes whitespace from the right end of a string. */
    trimRight(): string;

    /** Returns a copy with leading whitespace removed. */
    trimStart(): string;
    /** Returns a copy with trailing whitespace removed. */
    trimEnd(): string;
}

So Typescript is happy when used directly, and I suspect the ts-server (or tslint) in your neovim setup is using somewhat different configuration, or ignoring node context.

@isaacs
Copy link
Author

isaacs commented Apr 30, 2023

Dropping everything before 14 at least is a good idea.

Copy link
Member

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

I am ok with changing trim calls, but I do not want to change target in tsconfig.json without a clear idea what scenario that benefits.

- Use trimStart/trimEnd instead of deprecated trimLeft/trimRight
@isaacs
Copy link
Author

isaacs commented May 2, 2023

subsumed in #139, closing this one so I can make some changes on my main branch.

@isaacs isaacs closed this May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants