Using putout linter on xterm codebase part 2#3133
Using putout linter on xterm codebase part 2#3133Tyriar merged 4 commits intoxtermjs:masterfrom coderaiser:lint/putout
Conversation
jerch
left a comment
jerch
left a comment
There was a problem hiding this comment.
Thx for looking into that - looks good to me for most changes, but a few remarks below.
src/browser/Terminal.test.ts
Outdated
| describe('unicode - surrogates', () => { | ||
| it('2 characters per cell', function (): void { | ||
| this.timeout(10000); // This is needed because istanbul patches code and slows it down | ||
| this.timeout(10_000); // This is needed because istanbul patches code and slows it down |
There was a problem hiding this comment.
I think this notation is still way too new and we should not yet rely on it. Same goes for the other occurrences.
src/common/Platform.ts
Outdated
| const platform = (isNode) ? 'node' : navigator.platform; | ||
|
|
||
| export const isFirefox = !!~userAgent.indexOf('Firefox'); | ||
| export const isFirefox = !!userAgent.includes('Firefox'); |
There was a problem hiding this comment.
Since .includes already returns a boolean, the !! type conversion gets redundant.
src/common/tsconfig.json
Outdated
| "compilerOptions": { | ||
| "lib": [ | ||
| "es2015" | ||
| "es2017" |
There was a problem hiding this comment.
Whats the reason for ES2017? I have no strong obligations against ES2017, but would not like to change that for a non-obvious reason. Being more vanilla/old-school is not a bad thing in terms of supported targets while maintaining the same functionality.
There was a problem hiding this comment.
The thing is es2017 needs to be set to have ability to build files that contains includes with typescript.
There was a problem hiding this comment.
@coderaiser 👍 Ah ok, well I'm fine to move on to ES2017 then, but would like to hear @Tyriar and @mofux 's opinion in this regard first.
There was a problem hiding this comment.
My only thought is if we commit to this, that we also update all the other tsconfig.json files so they're not inconsistent. Since we only target evergreen browsers, I think this is safe to do?
There was a problem hiding this comment.
@Tyriar I think it will change certain output shims. Imho our codebase is not really affected by that (Was it around generators? Or promises with async/await? Cant remember...)
There was a problem hiding this comment.
Well that's for the target, the lib is just which typings to include. I see there is a lib "ES2016.Array.Include", that sounds like exactly what we want without needing to think about changing es2015.
There was a problem hiding this comment.
@Tyriar, I updated browser/tsconfig.json. Should I updatetsconfig.json in addons directories? They don't have lib option right now.
There was a problem hiding this comment.
@coderaiser no need if they don't use lib, does ES2016.Array.Include work instead of es2017?
src/common/tsconfig.json
Outdated
| "compilerOptions": { | ||
| "lib": [ | ||
| "es2015" | ||
| "es2017" |
There was a problem hiding this comment.
My only thought is if we commit to this, that we also update all the other tsconfig.json files so they're not inconsistent. Since we only target evergreen browsers, I think this is safe to do?
3 participants
Latest PR (#2953) was successfully merged, so I decide to do more checks with updated version of putout and discuss improving codebase with rules approved earlier and I can disable any of rules that not fits to current code style :).
So here is config:
{ "rules": { "strict-mode": "off", "remove-useless-spread/object": "off", "remove-iife": "off", "convert-generic-to-shorthand": "off", "remove-console": "off", "remove-unused-variables": "off", "remove-useless-variables": "off", "apply-destructuring": "off", "merge-if-statements": "off", "convert-apply-to-spread": "off", "convert-math-pow": "off", "convert-for-to-for-of": "off", "convert-template-to-string": "off", "remove-empty": "on", "convert-index-of-to-includes": "on", "convert-for-each-to-for-of": "on", "apply-shorthand-properties": "on", "apply-numeric-separators": "off" }, }