search

TypeStrong / atom-typescript

The only TypeScript package you will ever need
https://atom.io/packages/atom-typescript
MIT License
1.13k stars 205 forks source link

use new atom ide types #1559

Closed aminya closed 3 years ago

aminya commented 3 years ago

This uses the new atom-ide types which are provided in atom-ide-base.

aminya commented 3 years ago

@lierdakil When I run prettier nothing changes! Is this error a false-positive?

lierdakil commented 3 years ago

Apparently prettier is broken on good ol' Trusty. I've just updated Travis config to use Bionic. It should (hopefully) work if you merge/rebase.

aminya commented 3 years ago

Rebased it. Now it works 😄

lierdakil commented 3 years ago

Can we have a types-only package? It feels somewhat weird to install something just for the types. Especially considering atom-ide-base npm package is 3.6M when the types we need are only 44K (i.e. about 1.2%)

Yes, I know, common wisdom is to bundle types with the package. But here it's a bit different, since the absolute majority of atom-ide-base consumers won't intend to actually use any code in the package.

aminya commented 3 years ago

@lierdakil

Actually, I plan to add more functionalities to atom-ide-base. Since it is both an apm and an npm package. So in the near future, there will be some code that all the IDE packages would need to use. Many of the logics can be refactored in a commonplace.

I can remove the modules folder, which includes the legacy nuclide. That's the part which takes the most space. https://github.com/atom-ide-community/atom-ide-base/commit/e0beb4f842a08f49caf7542a71a1f3e30a5217bb

This should not affect the end-user for now. It is just a devDependency. 🤔 Does it? We have bigger packages in the devDependencies now like parcel which has a post install that builds a native module! If this is going to be installed in a prod install, we have a bigger problem!

aminya commented 3 years ago

Updated to atom-ide-base 1.4.1 which is smaller.

image

lierdakil commented 3 years ago

This should not affect the end-user for now.

It should not, unless something is very wrong, or the end-user decides they want to hack on the package. This is admittedly a minor nitpick.

Updated to atom-ide-base 1.4.1 which is smaller.

Okay, cool, works for me.

lierdakil commented 3 years ago

Merged manually due to conflicts.