aminya commented 4 years ago

This is a work in progress, so feel free to give suggestions, feedback, or engage!

Adding type definitions

old PR

### Description of the Change - Release Notes - [X] Converting codebase source to typescript while being completely backward-compatible - Fixes https://github.com/atom/etch/issues/80 - [x] getting `index.d.ts` to expose the types so `import {dom} from 'etch'` gives `dom`'s information in IntelliSense ## Maybe later: - Adding type definitions - Optimizing the code wherever possible # Verification Process - I verify the changes against other packages that use etch: for example, [original atom-select-list](https://github.com/atom/atom-select-list) and the [my typescript atom-select-list](https://github.com/aminya/atom-select-list/tree/TypeScript). [atom-indent-detective](https://github.com/aminya/atom-indent-detective/tree/TypeScript) uses atom-select-list. - The tests are untouched to show backward compatibility

How to review?

Use the commit history instead of going through the files: https://github.com/atom/etch/pull/82/commits

Possible Drawbacks

Nothing (backward compatible - zero change in the API and JavaScript code)

Alternate Designs

using @types/etch. But this doesn't allow us to use the power of typescript for optimizing etch and catching the bugs.

aminya commented 4 years ago

@smhxx @GlenCFL @lierdakil @hackape Could you guys review this and help me with this?

lierdakil commented 4 years ago

It's going to be extremely hard to give a proper review. I would suggest doing this in stages. For instance, stage 1: just add typescript (non-strict mode). This should allow to simply rename js files and add minimal type annotations. Stage 2: add proper types. Self-explanatory. Stage 3: fix stuff uncovered during the first two stages.

Also, side note. Since this is an npm project, it's generally considered to be a good practice to exclude generated files (js and d.ts) from version control. And to exclude TS sources from npm (via npmignore).

aminya commented 4 years ago

It's going to be extremely hard to give a proper review. I would suggest doing this in stages. For instance, stage 1: just add typescript (non-strict mode). This should allow to simply rename js files and add minimal type annotations. Stage 2: add proper types. Self-explanatory. Stage 3: fix stuff uncovered during the first two stages.

I remade the commit history now.

I will keep compile commits separate from editing commits so we can drop them easily if we want to later using rebase.

Also, side note. Since this is an npm project, it's generally considered to be a good practice to exclude generated files (js and d.ts) from version control. And to exclude TS sources from npm (via npmignore).

I don't think this is a good idea:

Map files+ TS files allow debugging
Declaration files add IntelliSense for using this library. Excluding these files means that this package is like a plain JS package.

lierdakil commented 4 years ago

I don't think this is a good idea

You're missing the point. The point is to avoid including generated files into version control. Npm is not like Atom packages, your npm package can have different/additional contents than the repo. Hence, generally, we don't check in generated content into the repo (because it's easy to regenerate when needed), and we don't include source TS into the npm distribution (i.e. lib_src), because it's not used directly. Does this make sense? If not, consider how it's done on https://github.com/TypeStrong/ts-node, or indeed, https://github.com/Microsoft/TypeScript. You won't find generated JS in repo there.

aminya commented 4 years ago

I don't think this is a good idea

You're missing the point. The point is to avoid including generated files into version control. Npm is not like Atom packages, your npm package can have different/additional contents than the repo. Hence, generally, we don't check in generated content into the repo (because it's easy to regenerate when needed), and we don't include source TS into the npm distribution (i.e. lib_src), because it's not used directly. Does this make sense? If not, consider how it's done on TypeStrong/ts-node, or indeed, Microsoft/TypeScript. You won't find generated JS in repo there.

Exactly because etch is an npm package, we should include d.ts files. etch isn't used directly and doesn't run anything on its own (using JS files), but it is always a dependency for other packages (always for developing other packages).
When other packages want to use etch, they need to call the API using the declaration that is provided in those d.ts files. If we don't ship this with the package, then this package is equal to a plain package. Read the handbook to learn more about this: https://www.typescriptlang.org/docs/handbook/module-resolution.html:

At this point, the compiler will ask “what’s the shape of moduleA?” While this sounds straightforward, moduleA could be defined in one of your own .ts/.tsx files, or in a .d.ts that your code depends on.
They don't get IntelliSense/type descriptions when they call the functions. They will get an error that the package doesn't have the declaration files.
The ts-node and Typescript package that you mentioned have a compiling stage as their installation. But atom doesn't ship with this compiler, and we don't want to rely on the user for building. This will add a permanent TypeScript dependency to the package.
Regarding TS files and mappings, shipping them helps to debug the code + developing.

All in all, I have remade history, and it is very easy to follow now.

lierdakil commented 4 years ago

You're missing the point again. I'll repeat this one more time: you don't need to have generated files in the Git repo to have those in the NPM package. Prepublish script and .npmignore rules will take care of building *.js and *.d.ts and uploading those to npm. Here's a random tutorial on using TS with NPM (literally the first one I found on Google): https://itnext.io/step-by-step-building-and-publishing-an-npm-typescript-package-44fe7164964c#2027. Most would agree that tracking generated content in the VCS isn't a great practice.

hackape commented 4 years ago

Hi @aminya. Basically lierdakil is saying you mistake this git repo for the actual npm package. The contents of the published npm package is NOT identical to what presents here in this git repo.

As per his suggestion "doing this in stages", you might follow these steps to create a cleaner commit history.

start with your second commit "add tsconfig/tslint + Add dev dependencies"
rename lib/*.js to src/*.ts without changing any files' content.
now you change the code from plain js to actual ts, ideally the diff of this commit should reflect clearly that most modification is just type annotation.
from here, whatever left that needs to be fixed follows

At this point you should've successfully converted all .js files to .ts ones. Ideally no more .js files should present in this repo.

As for npm package. .js files and .d.ts still present, but they are codegen'ed and .gitignored, so no show in this repo. On the other hand, .ts files exist only in this repo but don't show in the npm package, they should be .npmignored.

I hope this clears up the problem.

hackape commented 4 years ago

Side note, I think for this repo, an index.d.ts suffices a good PR, or maybe you just publish a @type/etch. Maintainers are generally reluctant to switch tool chain.

aminya commented 4 years ago

Hi @aminya. Basically lierdakil is saying you mistake this git repo for the actual npm package. The contents of the published npm package is NOT identical to what presents here in this git repo.

As per his suggestion "doing this in stages", you might follow these steps to create a cleaner commit history.
1. start with your second commit "add tsconfig/tslint + Add dev dependencies"

2. rename `lib/*.js` to `src/*.ts` without changing any files' content.

3. now you change the code from plain js to actual ts, ideally the diff of this commit should reflect clearly that most modification is just type annotation.

4. from here, whatever left that needs to be fixed follows
At this point you should've successfully converted all .js files to .ts ones. Ideally no more .js files should present in this repo.

As for npm package. .js files and .d.ts still present, but they are codegen'ed and .gitignored, so no show in this repo. On the other hand, .ts files exist only in this repo but don't show in the npm package, they should be .npmignored.

I hope this clears up the problem.

Thank you for the instructions! It is really helpful.

aminya commented 4 years ago

Side note, I think for this repo, an index.d.ts suffices a good PR, or maybe you just publish a @type/etch. Maintainers are generally reluctant to switch tool chain.

Yeah, that seems a faster way to get type definitions into work, but will make it hard to maintain two places. Although this package doesn't change much. It is in a stable mode now. Also, using TypeScript we can improve the package itself.

aminya commented 4 years ago

By following commit history you can see the changes.

Renaming is done in https://github.com/atom/etch/pull/82/commits/8cf2063422ab929b72c938f87a32bce34d9e6aed

I followed your advice and I added lib to files entry of package.json (in https://github.com/atom/etch/pull/82/commits/0db4551dded2e44a69b3d734819003f1b3174617).

Regarding adding lib to gitignore, I believe it is not possible to do that unless I delete the js source files! Should I do it right now?

Also, I have no idea why Travis fails on https://travis-ci.org/github/atom/etch/builds/666076960#L722

aminya commented 4 years ago

We kept talking about the tooling and less important stuff, but no one gave me feedback on the actual types. Does anyone have any feedback about them?

@BinaryMuse @nathansobo

lierdakil commented 4 years ago

Sorry, didn't find time to do the review proper (yet). One thing that jumped at me is the amount of any in tag definitions. I think it should be possible to do a little better.

Also note, HTML5 custom elements are a thing, so IntrinsicElements probably needs to include the catch-all [tag:string] key.

I did write an etch typing at some point way back when, and while it's by far not ideal, you might be able to borrow something useful from it (or at least I hope so): https://github.com/atom-haskell/typings/blob/master/etch/index.d.ts

DeeDeeG commented 4 years ago

Just popping in to say, if you want passing CI, take a look at #85. It updates Travis CI to run X Virtual Framebuffer correctly, so Atom can be tested in the headless CI environment. Required for the app to run in CI, much less pass tests.

Alternative: Pin the CI environment to Ubuntu Trusty, by adding dist: trusty in .travis.yml.

I won't attempt to review this, as I have no knowledge of TypeScript.

aminya commented 4 years ago

This branch is a little out of date. I need to redo the work.

I was thinking of adding the types in d.ts files and do not edit the JavaSript files. This way we can get TypeScript features without changing the code. It is quite hard to convince the developers to switch to typescript totally.

aminya commented 4 years ago

This branch is backed up at https://github.com/aminya/etch/tree/typescript.

I will create a separate PR which only adds types.

atom / etch

Adding Types #82

How to review?

Possible Drawbacks

Alternate Designs