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

migrate tslint to eslint #459

Merged
merged 14 commits into from
Jan 9, 2022
Merged

migrate tslint to eslint #459

merged 14 commits into from
Jan 9, 2022

Conversation

tjx666
Copy link
Contributor

@tjx666 tjx666 commented Dec 19, 2021

  1. migrate tslint to eslint
  2. fix eslint lint errors
  3. replace pretty-quick with lint-staged
  4. fix some ts compile error

But I occured an issue that I can't activate extenison. This issue is related with module code-block-writer which is dependent by ts-morph

My version info:

Version: 1.64.0-insider (Universal)
Commit: d12df34e31b4a018735d312a8947d79331132368
Date: 2021-12-17T05:14:21.805Z
Electron: 13.5.2
Chromium: 91.0.4472.164
Node.js: 14.16.0
V8: 9.1.269.39-electron.0
OS: Darwin x64 21.2.0

image

@tjx666
Copy link
Contributor Author

tjx666 commented Dec 19, 2021

This project compile costs too long. One reason maybe whenever this ts project contains all test files. If I have free time, often weekend, I may help to do some more optimization to this project.

@nicoespeon
Copy link
Owner

Thanks for helping @tjx666. I will have a look at that too. I noticed that error recently, and I'm not sure where this is coming from. The file does exist, so that's weird.

cc @fabien0102

@@ -20,19 +20,19 @@ export class GildedRose {
updateQuality() {
for (let i = 0; i < this.items.length; i++) {
if (
this.items[i].name != "Aged Brie" &&
this.items[i].name != "Backstage passes to a TAFKAL80ETC concert"
this.items[i].name !== "Aged Brie" &&
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, the playground is intentionally invalid. We should exclude this folder from the linter.

The playground code is the one that will open in the Extension Host when you debug locally.

@@ -40,7 +40,7 @@ async function extractGenericType(editor: Editor) {

function findAllOccurrences(ast: t.AST, selection: Selection): AllOccurrences {
let selectedOccurrence: Occurrence | null = null;
let otherOccurrences: Occurrence[] = [];
const otherOccurrences: Occurrence[] = [];
Copy link
Owner

Choose a reason for hiding this comment

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

👍 Nice, thanks for fixing all of these

onMatch: (path: t.NodePath<t.VariableDeclaration>) => void
): t.Visitor {
return {
VariableDeclaration(path) {
selection;
Copy link
Owner

Choose a reason for hiding this comment

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

👍 Good catch.

@@ -13,7 +13,7 @@ async function renameSymbol(editor: Editor) {
}
}

function findSelectedSymbol(editor: Editor): Symbol {
function findSelectedSymbol(editor: Editor): Nothing {
Copy link
Owner

Choose a reason for hiding this comment

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

That doesn't seem right. result may be an Identifier. Both are implementing the Symbol interface, so the previous code should have been correct. What was the linter issue?

@@ -108,8 +108,7 @@ export class TypeChecker {
program: ts.Program
): ts.Node | undefined {
try {
// @ts-ignore Internal method
return ts.getTouchingPropertyName(
return (ts as any).getTouchingPropertyName(
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm I'd rather keep a @ts-expect-error here, explaining it's an internal method we are using. I don't like parsing stuff to as any because it bypasses type checking. @ts-expect-error leaves the door open for the problem to be solved in the future.

@@ -10,7 +10,6 @@ src/**
.adr-dir
.gitignore
**/tsconfig.json
**/tslint.json
**/*.map
**/*.ts
wallaby.js
Copy link
Owner

Choose a reason for hiding this comment

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

We may need to revisit this list to make sure we don't bundle configuration files with the new setup.

package.json Outdated
"glob": "7.2.0",
"husky": "7.0.4",
"hygen": "6.1.0",
"jest": "27.4.2",
"lint-staged": "^12.1.3",
Copy link
Owner

Choose a reason for hiding this comment

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

🍰 I prefer to omit the caret and install exact versions of packages. That leaves no room for surprises. The JS world isn't very SemVer friendly, so I had surprises in the past with flexible versions being installed.

We have good tests and dependabot regularly makes sure we keep up-to-date, so that's not an issue.

"no-empty": OFF,
"no-use-before-define": OFF
}
};
Copy link
Owner

Choose a reason for hiding this comment

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

👍 Nice!

@nicoespeon
Copy link
Owner

@tjx666 can you fix the conflicts and address the few comments I left?

On my side, I'll investigate to fix the code-block-writer issue when activating the extension + try to make the project compile faster.

@tjx666
Copy link
Contributor Author

tjx666 commented Jan 6, 2022

Fix them this weekend

@nicoespeon
Copy link
Owner

@tjx666 FYI, the "Cannot find module './comment-char.js'" was a bug caused by the upgrade of ts-morph to v13. I fixed that on the main branch (commit 43ed32f).

That doesn't optimize the perfs, but at least it works now 👍

@tjx666
Copy link
Contributor Author

tjx666 commented Jan 8, 2022

@nicoespeon fixed

@nicoespeon
Copy link
Owner

@tjx666 working like a charm! Thank you for this super cool upgrade ❤️

@nicoespeon
Copy link
Owner

@all-contributors please add tjx666 for infra

@allcontributors
Copy link
Contributor

@nicoespeon

I've put up a pull request to add @tjx666! 🎉

@nicoespeon nicoespeon merged commit 709e9c7 into nicoespeon:main Jan 9, 2022
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.

None yet

2 participants