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

Flow/TypeScript #323

Open
ghost opened this issue Feb 16, 2018 · 37 comments · May be fixed by #651
Open

Flow/TypeScript #323

ghost opened this issue Feb 16, 2018 · 37 comments · May be fixed by #651

Comments

@ghost
Copy link

ghost commented Feb 16, 2018

Explore these tools

@demipixel
Copy link
Contributor

demipixel commented May 23, 2018

  • minecraft-data (PR submitted)
  • minecraft-protocol (Maybe? Types for the objects to client.write would be nice but I don't know if it's possible based solely on the string)
  • prismarine-chunk (Should be easy, as should the following)
  • prismarine-entity
  • prismarine-item
  • prismarine-world
  • prismarine-windows
  • prismarine-provider-anvil

Then we have the non-prismarine stuff, which we made need external typings for

  • vec3
  • random-seed
  • moment has its own type defs
  • emit-then
  • event-promise (there's this which looks much more, dare I say it, promising than our current library. It has types already and an actual README and such, so maybe we should switch?)
  • diamond-square (Owned by rom, very basic API, should be fine)
  • flatmap is used in one test, I doubt we need it for this
  • mkdirp is also used in one place
  • node-uuid is deprecated, apparently we should be using uuid which has types
  • range, similar to flatmap, only used in one spot. Maybe it might be worth creating basic lib functions for these things, since I don't think we should make people install packages for such basic function (also, range could just be done with [...Array(100)].map(i => i)
  • spiralloop is mine, but again, it's only 50 lines, does it deserve its own package? Especially since it's used in one place.
  • request
  • request-promise

We could add the devDependencies but I think this is enough for now 😉

Of course, we don't have to do all of these, just some stuff to think about. I can at least try to do the prismarine ones.

@demipixel
Copy link
Contributor

Also, we need to decide what bundler to use. Webpack is popular but I'm not sure if it's the best for the job.

@rom1504
Copy link
Member

rom1504 commented May 23, 2018

Why do we need a bundler ? I thought the types were just for the ide to autocomplete stuff and for tools to check correct typing.a

@demipixel
Copy link
Contributor

Well it's not regular JS, so at the very least you need to compile the ts files into js ones (or a single js one).

@rom1504
Copy link
Member

rom1504 commented May 23, 2018 via email

@demipixel
Copy link
Contributor

You can't run TS files, you have to convert them to JS files and run those (although using stuff like watch you can automate that).

@rom1504
Copy link
Member

rom1504 commented May 23, 2018 via email

@demipixel
Copy link
Contributor

Well we were talking about converting the project to TypeScript. Manually handling the typings means any time you change something in the API, you have to remember to change it in the typings.

@rom1504
Copy link
Member

rom1504 commented May 23, 2018

No I don't think we should convert the project to typescript.
I thought typescript types could be used along with JavaScript code.

@demipixel
Copy link
Contributor

Well, you can write a typings file and manually handle it from that. In addition, flow provides the option to add commented typings.

@rom1504
Copy link
Member

rom1504 commented May 23, 2018

What do you mean with manually handle it ? We don't need a bundler then.

@ghost
Copy link
Author

ghost commented Jun 3, 2018

TypeScript just has it's own tsc command that (might?) work fine. Although we may want to use something like webpack so we can set up a development and production profile.

@rom1504
Copy link
Member

rom1504 commented Jun 3, 2018

I don't think we should convert PrismarineJS stuff to typescript.
Adding types in separate files, ok, but if we need to convert the code files to ts I don't think it's a good idea.

@ghost
Copy link
Author

ghost commented Jun 3, 2018

No, the prismarine modules do not need to be converted.

@ghost
Copy link
Author

ghost commented Jun 3, 2018

Remember, though, that we don't need to use TypeScript, we could always use Flow and flow-remove-types which just simply removes the typings from the file producing the same JavaScript. As well, we can convert TypeScript typings to flow definitions https://github.com/bcherny/flow-to-typescript and by nature, Flow is less intrusive than TypeScript as everything doesn't need to be converted and you can just omit types if you wish.

@ghost
Copy link
Author

ghost commented Jun 3, 2018

Also, we can just include the typings for prismarine inside of the flying-squid repository as that is normal for most flow projects to do (this means we don't need to add typescript typings to each and every repository)

@rom1504
Copy link
Member

rom1504 commented Jun 3, 2018

so even with flow we would need to compile files ?

@ghost
Copy link
Author

ghost commented Jun 3, 2018

I mean... there is https://flow.org/en/docs/types/comments/. But I really don't want to use that. As with every typings system, it's obviously non-standard JavaScript, but flow-remove-types is really lightweight and just simply strips files of types and writes them to dist/. The announcement is here: https://flow.org/blog/2015/02/20/Flow-Comments/

@rom1504
Copy link
Member

rom1504 commented Jun 3, 2018

Ok I understand. Then I guess we can just not add types.
We finally got rid of all the complexities of babel/glup, I think it's better if we don't reintroduce it.

@CertainLach
Copy link
Contributor

Typescript can be used with babel: https://babeljs.io/docs/en/next/babel-plugin-transform-typescript.html

@CertainLach
Copy link
Contributor

Also, typings can be written in separate .ts.d files, but this is harder to maintain (Need to change both js and .ts.d for i.e adding/renaming/removing method/field)

@himself65
Copy link

@CertainLach

room1504 has indicated that they dont want to use babel

@hktr92
Copy link

hktr92 commented Dec 25, 2019

i would use luxon, it has cleaner api and simpler usage.
we don't need to bundle stuff, we can write directly in typescript and transpile it to js using tsc. node has support for es6 modules since v12. typescript has a target for build, and know how to transpile for cjs (thus no need for babel)

and for packaging, i would recommend zeit/pkg and it should suffice. it generated a single binary for the whole package. use it with uglifyjs and it's all set.

@demipixel
Copy link
Contributor

Since I last participated in this thread, I use typescript for all the things that are practical, and so I think TypeScript for this project would be a great idea...

On the other hand, there's plenty of existing issues and missing features for this project, so I'm not sure if a lot of development work should be exhausted on this at the moment.

@Pandapip1
Copy link
Contributor

Any updates on adding .d.ts typings?

@rom1504
Copy link
Member

rom1504 commented Jan 6, 2024

No but you can add them if you want

@zardoy
Copy link
Contributor

zardoy commented Jan 7, 2024

Any updates on adding .d.ts typings?

Technically they are here.

@rom1504 We can auto-generate first-class typing from here https://github.com/zardoy/space-squid/tree/everything/src/lib and since its in TS it guarantees that typings are correct and .d.ts will have all things covered since the codebase itself reuses the typings. I don't really know what is hardforking, but I'm always open to discussing the changes :) As I remember it was painful and not fun for me to work in js code without this:

At that time I used a chance to show someone else how to add typesafety & types to a big js project with almost fully automated scripts, I wouldn't do it now but I also don't see the point of reverting that work

I enjoy the fact that I can ctrl+click on absolutely any symbol and see its source. I like it when everything is structured and if I remember correctly it wasn't possible to use js+d.ts setup to fit the typings at that level of incapsulation, since TS is known to have a massive number of limitations in non .ts files unfortunately

if you have any ideas for improvements I would be glad to hear them out!

All in all, I would never go with the way mentioned in #579 (comment)

I generate typings & docs & useful hints and completions just from .ts, but what cons of doing this?

Anyway, I'm not forcing anyone into anything, just want to hear someone else thoughts about the technical changes

@Pandapip1
Copy link
Contributor

Personally, I'm a fan of using regular typescript for everything. It's a minor compile-time step, but the advantages of having a strong type system cannot be overstated.

@zardoy
Copy link
Contributor

zardoy commented Jan 10, 2024

Personally, I'm a fan of using regular typescript for everything. It's a minor compile-time step, but the advantages of having a strong type system cannot be overstated.

I don't feel like I'm a TS fan. I have super strong opinions about having it in big projects that mean to be used by other users (that's why sometimes quick dev scripts don't benefit from using it). The only goal is to ensure that the project reuses provided typings. I think the most efficient solution is having types with jsdoc instead of classic .md documentation. Just imagine writing a type in ts is like d documenting a new prop/method in a markdown file, but it also forces you to write it correctly. For example, many useful methods were undocumented here.

I don't think you need a strong typing system to benefit from autocomplete & error checking. I used only one generic in the rewrite - for the Command class because it had nested logic.

On the other hand, my personal opinion is that if you have a strong types system you can ensure that almost all operations (eg method calls) are 100% semantically correct which eliminates 99% of actual runtime errors.

Also, you can skip the compile step by using tsx instead of node or even better - bun. Bun was actually fun to try there as I also managed to add a proper hot reload. Just imagine: you edit flying squid and don't need to restart the server / do relogin.

I'll try first to publish it to npm to see how well other dev tools work with the module-scoped typing system and then try to find time to open PR to gain additional feedback on the actual per-file changes. any suggestions are welcome

@Pandapip1
Copy link
Contributor

On the other hand, my personal opinion is that if you have a strong types system you can ensure that almost all operations (eg method calls) are 100% semantically correct which eliminates 99% of actual runtime errors.

This is the biggest reason why I want this. It catches so many errors. TS has almost certainly saved me at least a couple dozen hours of debugging.

Also, you can skip the compile step by using tsx instead of node or even better - bun. Bun was actually fun to try there as I also managed to add a proper hot reload. Just imagine: you edit flying squid and don't need to restart the server / do relogin.

Hello fellow bun user!

@rom1504
Copy link
Member

rom1504 commented Jan 10, 2024

Discussion can be moved here PrismarineJS/prismarine-contribute#7

My opinion is still that it's bad as it encourages big projects instead of small well defined packages

@rom1504
Copy link
Member

rom1504 commented Jan 10, 2024

But also why debate, if you feel like it's useful for some existing project, try to rewrite, send a PR and show that it's better than the previous version

@zardoy
Copy link
Contributor

zardoy commented Jan 10, 2024

But also why debate, if you feel like it's useful for some existing project, try to rewrite, send a PR and show that it's better than the previous version

You can remember that prismrine web client pr 😅 it won't be an incremental change, but I'll do

My opinion is still that it's bad as it encourages big projects instead of small well defined packages

I believe these are unrelated things. It encourages better quality project by having you think of a type system, but once you have it setup I don't think it's a big problem maintaining it. I see your point: you don't want to deal with possible TS errors and instead focus on actual logic implementation, but you better spend time on having the type system & figuring out how to avoid weird ts errors rather than writing the types yourself. Once you are familiar with TS gotchas it won't take much time (that's why I personally moved it to TS instead of writing type declarations myself) and for some contributors, it would be much easier to work on the project because it allows to use much more powerful developer toolchain. I think you should be flexible with TS and use it at your skill level e.g. you would better have "any"s in your codebase rather than not using type system at all. Flying squid is already a big project with a lot of modules & linked methods that are called across modules and it's easy to forget them all. That's why TS just perfectly fits in this type of project. It's much easier to do internal refactoring and bootstrap new modules when you have the docs integrated right into your code. For the same, I think it would be much easier to play around with the project for new contributors since they would be able to ctrl+click on any method / property to see its source and hundred of times mentioned autocomplete (including from those ai-based tools). I also got used to it and it's wild for me to not have this option. Other contributors who don't care about TS and want to focus on js only can skip dealing with types and disable errors at all, but if you mean to ship the code to end users you should have better code quality checks

@rom1504
Copy link
Member

rom1504 commented Jan 10, 2024

You can remember that prismrine web client pr 😅 it won't be an incremental change, but I'll do

I think it should be a PR that is limited to changing to typescript and show the value of it.

There's been a ton of debate on this in PrismarineJS but basically nobody trying to prove it's useful

@rom1504
Copy link
Member

rom1504 commented Jan 10, 2024

squid is already a big project with a lot of modules & linked methods that are called across modules and it's easy to forget them all.

I agree, it's too big. Would be great to make it smaller

@rom1504
Copy link
Member

rom1504 commented Jan 10, 2024

My best idea to make it smaller (and also mineflayer) is PrismarineJS/mineflayer#334

I'm seeing a lot of people use typescript to do "like java" codebases and that's one of the problems with it

If people were coding like Haskell or ocaml coders then it would be different

@zardoy
Copy link
Contributor

zardoy commented Jan 10, 2024

There's been a ton of debate on this in PrismarineJS but basically nobody trying to prove it's useful

It's extremely useful if you know how to use it. I'm not sure what we can count as proof, I run another project that is on ts and it saves me a lot of time so I can implement things faster. Let's see a pr...

I agree, it's too big. Would be great to make it smaller

Also I think there is nothing bad in that its big, yes, some things like redstone logic and so on should be refactored into independent modules, but it will stay big and will still have a lot of methods / other things. The project just has a big purpose.

If people were coding like Haskell or ocaml coders then it would be different

Agree, I'm not saying ts is ideal, but its so much better than nothing. And no, java is different. In java you have different requirements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants