Update dependencies to latest version + add ts-standard linting rules

[theoludwig]

The purpose of this pull request (and checklist)

• Update dependencies to latest (typescript included)
• Add ts-standard linting rules
• Upgrade the vscode field in the package.json to ^1.50.0 (instead of ^1.16.0)
• Add npm run lint script to run with ci integration (travis here)
• Fix the errors thrown by ts-standard

Description

This is a huge pull request with lot of changes since the code is very old and outdated (package-lock.json changes for example), especially since this pull request will follow standard rules, no 4 spaces, no semicolons for example.

Which issue (if any) does this pull request address?

closes #130 , closes #124, closes #125, closes #133

[feross]

Looks good so far! Thank you for taking on this task, @divlo.

Can we please do this in stages? It will be a lot easier to review changes if we don't mix formatting changes with code changes. If they're mixed together, it's too hard to review the code changes. I'd like to land this PR since it already improves things quite a bit. Would you like to get it into a mergeable state and then focus on the rest in additional PRs?

[theoludwig]

Right, doing multiple pull requests instead of big one will be lot easier to review.

The real problem with this pull request is that in its current state, I installed ts-standard and I've got many linting errors, should I fix it in this PR or merge that one in master and do another PR to fix linting errors ?

Also before merging this PR to master, I still need to check that nothing break, I only tested with standard engine. Since there are not automated tests yet, we need to manually test if everything is working same way as before.

For the next tasks, I'll definitely do smaller pull request.

[feross]

This is looking really good so far!

@divlo The real problem with this pull request is that in its current state, I installed ts-standard and I've got many linting errors, should I fix it in this PR or merge that one in master and do another PR to fix linting errors ?

Feel free to fix it in this PR. As long as you do it in a separate commit, I can just review it separately from the rest of the commits. Once you've done that and tested the changes, can you unmark this as a draft? Then I'll merge.

Also before merging this PR to master, I still need to check that nothing break, I only tested with standard engine

Yep, looking forward to your future PRs where you add testing 🤞

[feross]

@divlo Your latest changes look good. Let me know when you're ready to merge this by unmarking it as a draft PR.

[theoludwig]

Thanks for your support @feross! 😀
Sure, I'll let you know when it's ready.

It's a tedious and repetitive work to fix one by one linting errors on a project while taking into account that everything should work same way as before. It doesn't really improve the extension itself (doesn't add features or fix bugs) but it will be easier to maintain later on, catch style issues and programmers errors early, as it's one of the goal of standard.

Current PR state: There are still lint errors thrown by ts-standard :

• ~ 100 inside the server folder.
• ~ 40 inside client folder.

I will try to fix more of those this weekend. Anyone feel free to add commits to this PR and help to fix lint errors by running npm run format either in client or server directory.

[feross]

@divlo Thank you for doing this work – as a newly converted VSCode user, I seriously appreciate it. 👍

If you want to split up this work, we could remove ts-standard from the npm test script and we can fix the rest up in future PRs. Up to you!

[theoludwig]

@LinusU @toddbluhm Feel free to help to fix the linting errors, so we can merge this to master faster.
Indeed this pull request close #133

I have trouble fixing some linting errors since I didn't write the code so I need to understand how the code works before changing something.

@feross I guess now that we started to do the linting work, we should finish it.
There was no npm test or npm run lint scripts or such before this PR.
Also I added the npm run lint script to run with travis.
As I added travis we can now see the current PR state : https://travis-ci.org/github/standard/vscode-standardjs/jobs/743192634

Current PR state (errors thrown by ts-standard) :

• 96 problems inside server folder
• 38 problems inside client folder

There were issues with this rule : https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-namespace.md
I find out that an "easy" fix would be to create new files inside utils folder and export what is needed so I can do import * as myNamespace from './utils/myNamespace' so that's what I did.

[feross]

@LinusU @toddbluhm I would be super grateful if you could help @divlo with this PR! I'm not very experienced with TS, so I'm struggling to review some of these changes.

If we can land this PR soon then the VSCode editor plugin will be back in a "maintained" state and 300k users will be super grateful :) Eager to improve things!

[theoludwig]

In the client folder, there are 5 linting errors left to fix but it seems like we can't really fix them.

The first 3 errors are false positive since we're actually re assiging these variables but it's like 300 lines of code later.
I don't know might be an issue with ts-standard or eslint, should we ignore them with a comment like eslint-disable-next-line or any other workaround ?

The 4th error, folder will evaluate to [object Object] when stringified is a real error but I don't know what the code should do there, since we're mapping trough an array of folders to stringify them but what property of that object should we use or should we simply use JSON.stringify?

Last error, is because I did an async function inside a callback to use await keyword so I don't have the lint error no-floating-promise but the callback should return void, now since it's an async function, it returns Promise<void>, that's why it's complaining, in both cases I've got a lint error.

[feross]

I don't know might be an issue with ts-standard or eslint, should we ignore them with a comment like eslint-disable-next-line or any other workaround ?

Yes, feel free to use an ignore comment if the linter seems to have a bug.

The 4th error, folder will evaluate to [object Object] when stringified is a real error but I don't know what the code should do there, since we're mapping trough an array of folders to stringify them but what property of that object should we use or should we simply use JSON.stringify?

@LinusU @toddbluhm Do you have any idea what might have been intended here?

Last error, is because I did an async function inside a callback to use await keyword so I don't have the lint error no-floating-promise but the callback should return void, now since it's an async function, it returns Promise, that's why it's complaining, in both cases I've got a lint error.

You can fix this issue by keeping the outer function as a normal non-async function, and make a IIFE async function inside so you can use the await keyword. Something like this:

function () {
  ;(async function () {
    await somePromise()
    ...
  })()
}

[theoludwig]

Good news, I fixed all the linting errors. 🎉
Thanks again for your help. @feross

I packaged the extension and tested it, on one of my project and everything seems to work same way as before, my project use ts-standard with these .vscode settings :

{
  "standard.enable": true,
  "standard.usePackageJson": true,
  "standard.engine": "ts-standard"
}

Hopefully, I didn't break anything, we should also test with different engines and different settings to make sure everything works.
The PR is not anymore in draft mode and is ready to be merge into master, if everything is fine and working.

Regarding how I fix the errors :
The IIFE was a good idea but still didn't work since I would also need to await the IIFE to not get the lint error no-floating-promise, so what I did is to put a comment eslint-disable-next-line, since it's a false positive, I don't think we have any others way to fix it badly.

I tried my best to avoid using eslint-disable-next-line comments but sometime I guess I had no other choices.
Currently there are 9 eslint-disable-next-line comments in the code.

In my opinion : we could decrease this number and improve further the code quality in future PRs, but as it is now we could just ignore these errors and merge that PR to master, do you think it's a viable option ?

Also, if you like my work, I would be happy to maintain this vscode extension (if you could add me to the org). So I can manage issues and review PRs and we can continually improve the extension as I (and hopefully 300k users too) also enjoy using this extension in projects. 🚀

[toddbluhm]

@divlo I took a look at those eslint-disable lines and resolved all of them. Most of it was just making typescript happy. Typescript isn't always able to infer the correct types so you need to provide it the correct type information.

The vars used before defined error was correct (just picky), we just needed to set them to null first so they contained a value. It was a bit annoying I could not set them to undefined because it conflicted with another rule (no unnecessary undefines), so we might need to relax that rule a bit.

As for the async functions instead of the void functions. I just told typescript that we know what we are doing and cast them to () => void. Again typescript is not perfect and needs some guidance sometimes. While typescript was technically correct here, we understand that what we are doing is not normal so we told it to go away.

[feross]

Incredible work, @divlo! I and the whole @standard/team appreciate this massive effort to bring this extension back into maintenance! You're awesome! 🤩

I'll merge this PR now! Before I cut a release, I'd like at least 5 VSCode users to run the code on master and report no problems.

[welcome]

🎉 Congrats on getting your first pull request landed!

[feross]

@divlo You're now a member of the @standard organization! ❤️❤️❤️

[feross]

I opened an issue seeking testers before we publish the new version: #135