Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use new atom ide types #1559

Closed
wants to merge 5 commits into from
Closed

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Sep 26, 2020

  • there is no compiled file

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

@aminya
Copy link
Contributor Author

aminya commented Sep 26, 2020

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

@lierdakil
Copy link
Collaborator

lierdakil commented Sep 26, 2020

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
Copy link
Contributor Author

aminya commented Sep 26, 2020

Rebased it. Now it works 😄

@lierdakil
Copy link
Collaborator

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
Copy link
Contributor Author

aminya commented Oct 1, 2020

@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.
atom-community/atom-ide-base@e0beb4f

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
Copy link
Contributor Author

aminya commented Oct 1, 2020

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

image

@lierdakil
Copy link
Collaborator

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
Copy link
Collaborator

Merged manually due to conflicts.

@lierdakil lierdakil closed this Oct 1, 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