Type script

[aminya]

Ready to get merged - Tested and works as expected

• Converted the codebase (JS/CS) to TypeScript.

• Changed atom-space-pen-views to atom-select-list

• Defining Selector Class instead of module

• using a StatusBar Class instead of module
  Fixes Uncaught TypeError: Cannot read property 'nodeName' of undefined #15 and Fixes Uncaught TypeError: Cannot read property 'webkitMatchesSelector' of undefined #16

• Optimized the code.

  • various optimizations
  • Optimized for loops using (see TypeScript-Optimization):
    • Not looking up arrays in for-head
    • Using ts-ransform for-of plugin

• build using Webpack

• Loading time is cut from 71ms to 0ms!

before:

after:

• Added configuration for TypeScript
• Added type declaration/annotations for some of the variables
• Added TODOs for improving the code
• Added Tests
• Added Benchmarks

/src includes the typescript source files (input).
/lib includes the compiled javascript (output).

• Squashed and remade the history based on @aviatesk suggestion.

[pfitzseb]

Neat, let me know when you think this is done.

Aside: I'd very much prefer no semi-colons -- they're not very useful in JS, and TS has even fewer corner cases where ;s do something.

[aminya]

Neat, let me know when you think this is done.

I am facing a problem with importing a class from atom-select-list:
https://stackoverflow.com/questions/60599391/calling-javascript-class-from-typescript-uncaught-typeerror-is-not-a-const

Do you know what is going on here?

[aminya]

Neat, let me know when you think this is done.

I am facing a problem with importing a class from atom-select-list:
stackoverflow.com/questions/60599391/calling-javascript-class-from-typescript-uncaught-typeerror-is-not-a-const
Do you know what is going on here?

I think this is fixed in 1b597d1, but I get a new error:
Error:

This is solved in bf753d4 🚀

[aminya]

@pfitzseb This is ready to get merged for now. 😃

• I have added some TODOs for future development.

PS: the name of the package doesn't match the name of the repository. If this isn't intentional, we should fix it.

[aminya]

This is very ready to get merged 😄 @pfitzseb

I fixed #15 by using a StatusBar Class instead of a module.

[aviatesk]

My thoughts on this:

• the changes are too big to be safely merged; in general it's better to split changes into small PRs as possible https://rangle.io/blog/the-art-of-humanizing-pull-requests-prs/
  • it's great that fixing Uncaught TypeError: Cannot read property 'nodeName' of undefined #15 but it's better to be done in a separate PR
• imho TypeScript would be not so fruitful but bothersome for this package
  • we need to sync the version of TS
  • this is such a small package that we can easily track its implementation and we don't really need to rely on type information; code would be verbose
  • TS should be introduced when really necessary, and in this case it's not imho
  • (well, I'm not just object to static-typing -- we use flow for Hydrogen for example, and I feel the type checking for a package in that scale is useful; my idea is about the balance between the verboseness/bothersome vs. usefulness of it)

[aminya]

My thoughts on this:

* the changes are too big to be safely merged; in general it's better to split changes into small PRs as possible [rangle.io/blog/the-art-of-humanizing-pull-requests-prs](https://rangle.io/blog/the-art-of-humanizing-pull-requests-prs/)
  
  * it's great that fixing #15 but it's better to be done in a separate PR

Thank you for your opinion.

Regarding your first point, I totally get your point regarding small PRs and I agree with it, however, this PR meant to fix the performance of this package, and for that, I had to rewrite different sections of the code, and edits in small chunks were either not possible or didn't help because of code overhaul (e.g. space-pen-views -> atom-select-list).

* imho TypeScript would be not so fruitful but bothersome for this package
  
  * we need to sync the version of TS
  * this is such a small package that we can easily track its implementation and we don't really need to rely on type information; code would be verbose
  * TS should be introduced when really necessary, and in this case it's not imho
  * (well, I'm not just object to static-typing -- we use [flow](https://flow.org/) for [Hydrogen](https://github.com/nteract/hydrogen) for example, and I feel the type checking for a package in that scale is useful; my idea is about the balance between the verboseness/bothersome vs. usefulness of it)

I want to disagree with this one though.

• You don't need to do anything regarding TS. This package is good to go and will act like a JS package for usage (JS is already built).

• Typescript isn't about just tracking the implementation. It brings many improvements over simple JavaScript (enhances JS without drawbacks):

  • it reveals many things that are not visible when you use JS. Things like type-changing of the variables, if a function returns the type we want, performance improvement by using better types, etc. You see this package loads 1/4 of the original, and its runtime was better in my measurements (30-50%).
  • IntelliSense/linting
  • API documentation on the fly. It allows others to understand what the code is doing.
  • You can write your normal JS code in TypeScript and it works. TypeScript infers many of the types automatically.
  • It removes the need for babel or any other processor.
  • We can compile TypeScript to Wasm using AssemblyScript which opens a whole new world for improving the package. For example, I plan to compile the getIndet function of this package.
  • The future isn't plain JavaScript. For example, VSCode is already using TS (which IMO has better performance while I like atom more).

Read this article to know more about the advantages:
https://dzone.com/articles/what-is-typescript-and-why-use-it

[aminya]

• I furthered improved the performance in bd320dd by defining a Selector class instead of a module and separating different independent functions from each other.

• Optimized for loops using (see TypeScript-Optimization):

  • Not looking up arrays in for-head
  • Using babel transform for-of plugin

• Minify using Babel (very minimalistic)

• Squashed and remade the history based on @aviatesk suggestion.

[aminya]

Could you the benchmark again? I cannot get reproduce your result but I just wanna see if it is improved on your computers.
@pfitzseb @aviatesk

On my system 5ms is reduced to 0 ms:

[pfitzseb]

I'm getting 0ms on both of my machines as well now -- is that just because package activation is deferred because of the activation hook?

[aminya]

I'm getting 0ms on both of my machines as well now -- is that just because package activation is deferred because of the activation hook?

Yes. That is why. But before 525aee1 still I got a very good loading time.

My inline benchmark code still gives 5ms. Probably Atom cannot measure the correct activation time especially when it is deferred.

My Github action shows 20ms in the online benchmark: https://github.com/aminya/atom-indent-detective/runs/561525274?check_suite_focus=true#step:6:83. That one is based on atom.packages.activate, which may not be as accurate as the inline benchmark.

---

The package loads much faster (compared to master branch) because:

• If you see the activate function, nothing happens there. Just a couple of functions are defined for atom:
  https://github.com/aminya/atom-indent-detective/blob/525aee15704119cb5b4f3d245caacda13a63dcf5/src/indent-detective.ts#L47-L109

• The only thing that may take a little time is making the status bar item:
  https://github.com/aminya/atom-indent-detective/blob/525aee15704119cb5b4f3d245caacda13a63dcf5/src/indent-detective.ts#L121-L124
  which you see the code is written using pure JS dom:
  https://github.com/aminya/atom-indent-detective/blob/525aee15704119cb5b4f3d245caacda13a63dcf5/src/status.ts#L10-L32

[aminya]

@pfitzseb Do you need anything else here from me?