Skip to content

Conversation

aminya
Copy link

@aminya aminya commented Mar 19, 2020

Ready to get merged

Please merge this so I can continue with other PRs

This PR doesn't change any behavior of Julia Client.

Initialization for Converting codebase to TypeScript:

This is the initialization of that PR.

I am distilling #704 into smaller PRs, so I can track the changes easier, and making reviewing the actual editing part easier for you.

All the code is stored in (lib_src) and doesn't change any source which is in use.

Config

  • adding configurations for future conversion
  • linting for TS, JS, Coffee (ESLint + TSLint)

@aminya aminya mentioned this pull request Mar 19, 2020
62 tasks
@aminya
Copy link
Author

aminya commented Mar 19, 2020

@pfitzseb @aviatesk Could you please merge this PR?

@aminya aminya force-pushed the typescript_init branch 3 times, most recently from af9dc50 to 700a1b4 Compare March 22, 2020 05:10
@aminya aminya force-pushed the typescript_init branch 2 times, most recently from 821c998 to c191a06 Compare March 28, 2020 12:43
@aminya
Copy link
Author

aminya commented Mar 28, 2020

I even simplified this PR more. I will do the whole process file by file in separate PRs, so you guys can continue developing without any conflicts.

So please merge this PR as soon as possible. This is blocking me from continuing.

aminya added 3 commits March 28, 2020 22:07
Update tsconfig.json

Update tsconfig.json
tslint - Turn of variable-name

Update tslint.json

Update tslint.json

Update tslint.json
Update package.json

Update package.json
@aviatesk
Copy link
Member

Hi @aminya, sorry for taking a time to get back to you.

We would very much welcome JSfying of our code base (in per-file basis), but I really don't appreciate conversions to TypeScript.

The main reason is that we're planning the migration of ink and julia-client in the coming months and it will be such a big refactoring.
At this point we don't want to give the other huge change to our code base but rather keep our code base as it has been, in order to perform the migration safely.

TS seems more promising for current front-end development in general, and it would be fancy to have type-safety and more extensive development utilities for Juno, in the future.
But this is clearly not our priority for now.
And to say more, I'm not sure TS will really help our code base, at least as far as we live with Atom:

  • type-safety provided by TS isn't strict/true -- the case is much worse especially when we have lots of dependencies that don't provide (reliable) type definitions (like etch for our case)
  • Atom doesn't support native TS compiler, and so TS will add another complexity for our development process
  • lots of development utilities provided by TS-server are available for JS files

@aminya aminya mentioned this pull request Mar 29, 2020
@aminya
Copy link
Author

aminya commented Mar 29, 2020

Hi @aminya, sorry for taking a time to get back to you.

Hi! No problem.

We would very much welcome JSfying of our code base (in per-file basis), but I really don't appreciate conversions to TypeScript.

The main reason is that we're planning the migration of ink and julia-client in the coming months and it will be such a big refactoring.
At this point we don't want to give the other huge change to our code base but rather keep our code base as it has been, in order to perform the migration safely.
TS seems more promising for current front-end development in general, and it would be fancy to have type-safety and more extensive development utilities for Juno, in the future.
But this is clearly not our priority for now.

Regarding this ink and julia-client merging, I have talked to @pfitzseb, and we said that it worth it to convert in the merging process. I would be happy to help merging Ink, but I think Julia-client itself can be enhanced on its own without mering Ink.

And to say more, I'm not sure TS will really help our code base, at least as far as we live with Atom:

* type-safety provided by TS isn't strict/true -- the case is much worse especially when we have lots of dependencies that don't provide (reliable) type definitions (like etch for our case)

* Atom doesn't support native TS compiler, and so TS will add another complexity for our development process

* lots of development utilities provided by TS-server are available for JS files

I already talked about the many benefits here: JunoLab/atom-indent-detective#17 (comment), but I didn't get any response from you:

  • 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

Last point:
Since I am doing the work for you (in a systematic way that is easy to follow), I don't really understand the reason for the objection and resistance.

@aviatesk
Copy link
Member

aviatesk commented Mar 29, 2020

I'm afraid to say, but did you really read my opinions ? What do you think on them ?

  • type-safety provided by TS isn't strict/true -- the case is much worse especially when we have lots of dependencies that don't provide (reliable) type definitions (like etch for our case)
  • Atom doesn't support native TS compiler, and so TS will add another complexity for our development process
  • lots of development utilities provided by TS-server are available for JS files

You should always try to see pros and cons of things. And since this change is really big one, I would like to figure out it is really necessary to be done.

@aminya
Copy link
Author

aminya commented Mar 29, 2020

  • type-safety provided by TS isn't strict/true -- the case is much worse especially when we have lots of dependencies that don't provide (reliable) type definitions (like etch for our case)
  • lots of development utilities provided by TS-server are available for JS files

I mentioned the answers to these in my points: TypeScript is just JavaScript. lack of type definitions doesn't break the code or doesn't mean that it is not compiled. It just means that the code will be copied (compiled) like it is a simple JavaScript.

The reason that you think this adds complexity is that you think I am changing JavaScript to C++ for example 😄 while I am just fixing the code issues itself.

P.S: I have already made a PR to etch. If they don't merge it, I will instead make a @types/etch in a day:
atom/etch#82
While we don't need this because of what I just said in the above paragraph.

  • Atom doesn't support native TS compiler, and so TS will add another complexity for our development process

There is a compiler: https://github.com/smhxx/atom-ts-transpiler. But we don't wanna rely on the user's computer for compiling anyways. It makes sense to deliver the ready JS files to the user instead.

@aminya aminya mentioned this pull request Mar 30, 2020
12 tasks
aminya added 4 commits April 1, 2020 05:57
Add json lint

Update package.json
update scripts

Update package.json
- copy js/coffee files to lib_src - decaffeinate . --loose - remove 
coffee files -  js-to-ts-converter .
@aminya aminya force-pushed the typescript_init branch from f51d8fc to 0ba7301 Compare April 1, 2020 10:57
@aminya
Copy link
Author

aminya commented Apr 1, 2020

I separated Github Actions part of this PR into another PR #712, the failure of the linter on Travis is because of this. After Github's actions merge, the CI will pass.

@aviatesk
Copy link
Member

So after all, I would like to decline you suggestions for migrating into TS. Reasons:

  • the additional complexity of transpiling, declaring types, etc will be very annoying for this package, at least at this stage when the code base is still under rapid development
  • we can't get type-safety after all; we have dependencies that don't provide (valid) type definitions, and the part with interaction with ink is all untyped

@aviatesk aviatesk closed this Apr 13, 2020
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