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

Generate Typings #15

Closed
wants to merge 3 commits into from
Closed

Generate Typings #15

wants to merge 3 commits into from

Conversation

jharrilim
Copy link

@jharrilim jharrilim commented May 31, 2019

Hi there, I have added typings generation for all of the libraries. I was unable to generate them for fo, ga, hu, and mk, so I did those by hand. A couple of syntax changes were made as well.

Edit: If the PR is too large to review due to the generated files, I can rollback the generated files so that you may generate them instead.

Summary

  • Generates type definitions (index.d.ts files) for all languages, and references them in the package.json.
  • JSDoc comment included in the js.
  • crawl.sh has color functions changed to the POSIX compliant format.

@wooorm
Copy link
Owner

wooorm commented Jun 3, 2019

Hi there! 👋

First of all: thanks for the work, I’m sure it took quite some time, and I appreciate that.

I’m going to review this in one comment here because the change set is so large, so I think 👇🏽 is more readable

  • bc6143d Make color funcs POSIX compliant — looks good!
  • a8cf719
    • Add typings — This is a big one. I’m not necessarily against doing this, but I have run into several problems with people adding types to non-TS libraries. TypeScript recommends against it, and I think I agree
    • Update generate script — I’m not so happy about refactoring code if it’s unneeded, especially if it would break support on older Node and the like. Although it’s a build script so it doesn’t matter too much
    • Include type comment in index.js — I’m not a fan of a) adding a comment that reiterates the code, and b) adding a JSDoc comment doing that as well!
    • use destructuring syntax and const/let — I also honestly don’t think using only new features of JS is making the file easier to read. Note that “old” JS is still JS, and still modern JS as well. Moving the module.exports is unneeded 🤷‍♂️

@jharrilim
Copy link
Author

jharrilim commented Jun 3, 2019

Hi there,

Thanks for getting back to me on this. I also feel uncomfortable creating typings manually. I'm also unsure if the template itself should be TypeScript, it would require a bit of extra work to process it, but it will also most likely garble a lot of the downstream code which also needs to be committed.

The JSDoc might be unnecessary since it can be done in the types file anyway. I do believe having this comment in either spots will still be helpful though. VSCode shows these comments when you hover over the imported item. That instantly available documentation is quite enjoyable IMO, and even though it looks redundant since it is reiterating the code, it adds an extra layer of helpfulness for when a user does not wish to drill down into the code.

The syntax updates I suppose might be considered superfluous. I assumed most users will be using this library from Node due to the fs and path imports, and at this point, Node 6 has reached EOL and all currently supported versions of Node have this syntax available. I also think distinguishing between read only consts and mutable lets does in fact raise the readability of the code. Those are small changes that give us a lot of useful information.

The style choices come directly from how NodeJS currently structures their code, for example, the path module: https://github.com/nodejs/node/blob/master/lib/path.js

All in all though, the only part that really matters to me here is being able to receive typings, as that will allow me to use this code within TypeScript without having to write my own declaration, and being able to do it from within this library means that the types can be automatically propogated down to the individual libraries without having to manually push a type declaration to their respective @DefinitelyTyped repos.

Thank you very much for this repo btw, it is pretty much a gold mine.

@wooorm
Copy link
Owner

wooorm commented Jan 24, 2020

Hey again! Sorry for letting this sit.

I’d rather not accept TS types because I’m personally not a TS fan and adding it would make it hard for me to maintain this project in the future, sorry.

TS suggests using DT to add types to non-TS projects, and I agree with their recommendation.

Thank you very much for this repo btw, it is pretty much a gold mine.

Thank you 😊

@wooorm wooorm closed this Jan 24, 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