Closed erikyo closed 2 months ago
@erikyo Thank you for your work!
I'm so unfamilliar with TypeScript I didn't even realize it requires a build step. That's a bummer. It seems the impact on the code base isn't too big though. Although I'd prefer the non-TypeScript changes be submitted in a separate PR/commit (renaming /lib to /src, typo's, package.json improvements,...). It would result in a very concise TypeScript-only PR.
However, before moving ahead with TypeScript, I'd very much appreciate @andris9 's feedback. He is the original author of this library and I'd like to know which approach he'd take.
@andris9 the guy of node mailer? very gladly!
About the requested changes give me the time and I'll see if I can do what you asked for... unfortunately some are strictly necessary to compile without errors or pass the tests, but I'll see what I can do
As I haven't been active with this project for years, I don't consider myself to have any special rights to decide anything about the project's future. I think such decisions must be made by active maintainers only.
Certainly, your comment, advice, or review is highly appreciated, as your expertise in the repository is invaluable. I acknowledge that the modification I propose may not be minor, but given the recent upgrade of the module to ES modules and the ongoing efforts to maintain its activity, I believe that this change, albeit potentially controversial, is worth submitting.
To be honest, the adjustment I suggest isn't excessively radical and ensures robust type checking.
in short, Is a matter of decision-making, so it would not be ideal to leave all the burden to @smhg
I didn't even realize it requires a build step. That's a bummer.
For the time being yes, but simultaneously, it's the approach that ensures secure typing. It's a worthwhile endeavor. If you're interested, I can incorporate other automations to streamline the process. For instance, I could implement a prepublish
script that handles tasks like clearing the lib folder, automatically building, and running tests.
It's essential to note that the compiled lib folder should no longer be published to github but only to npm (and, on other hands, the src folder isn't published on npm), check here
@erikyo yes, having a prepublish
script is definitely a good idea.
But, let's split up this PR if ok for you. I will add some comments in the code to clarify what I'm thinking about.
My current thinking: since I will be maintaining this library for the forseable future, I want to have the option to throw out types if I can't maintain them. It looks good currently, but I don't want to end up in a situation where I can't publish a bugfix because things don't run/build because of TS. Obviously I would rather first want to request your help when necessary to avoid this scenario.
let's split up this PR if ok for you
👍, and indeed sorry, I saw the review and indeed some edits were not required at all
I want to have the option to throw out types if I can't maintain them
at the current implementation types are only checked and generated using the typescript engine but the code isn't changed. i know that is a widely used repo, I will use white gloves
Obviously I would rather first want to request your help when necessary to avoid this scenario.
Obviously i'm here if you need!
I have updated as you asked but I have a couple of concerns, that we may want to wipe out in the next PR:
The first is about the consolidation of all files into a single root folder, avoiding the placement of index.js directly in the repository root. Given that we can direct the 'bin' script to ./lib/index.js
, there should be no issue with this approach. This consolidation aids in the import of definitions, as I've encountered difficulty setting up types because compiled definitions must be emitted into a separate folder from the sources. For this purpose, the @types/
folder is utilized, and the compiled files need to reference the index file in the root, similar to the files in 'lib'. Having everything centralized in one location would elegantly resolve these challenges.
jsdoc can be enhanced... definitions aren't wrong but typescript can provide a better type checking leveraging on that, you can give a try with tsc --checkjs
and you will get some thing like this 👇. Sincerely, I wrote to you because i had to deal with this repo and the definitions were not clear to me and it took me longer than necessary to use it. I wish this would not happen to others, and typescript is a real friend of developers in this case ;)
About the jsDoc i did another branch where i implemented strict js type checking reducing type check issues (from > 400 to less than 50), but there is still a lot of work to be done though (some sections are really difficult to type without typescript).
it should look like this: https://github.com/erikyo/gettext-parser/blob/eb797c68e64c86d0feda41d1043a94a6e859429e/lib/pocompiler.js#L22-L43
I also think in that way could close some issues such as the #29 that which I actually believe that is forced conversion between array and string, and other issues that are really related to js duck typing
I would like to point out that I just use the tests to make sure that everything is OK so I am pretty sure that everything works exactly as before
About the jsDoc i did another branch where i implemented strict js type checking reducing type check issues (from > 400 to less than 50), but there is still a lot of work to be done though (some sections are really difficult to type without typescript).
it should look like this: https://github.com/erikyo/gettext-parser/blob/eb797c68e64c86d0feda41d1043a94a6e859429e/lib/pocompiler.js#L22-L43
You already lost me with those. Let's first clean up the current code with a pre-TS PR if you want. That would reduce the changed files by a lot. This can also already be released immediatly. We can discuss types in a second step.
We can discuss types in a second step.
That the reason why i moved the changes into a separate branch (https://github.com/erikyo/gettext-parser/tree/enforcing-types), they are important to me and I would not like to lose them, but I understand that they are not part of the subject of this PR.
I will create a dedicated pr for that, where we could address the transition to es2015 by creating classes and their related types
I did a small test with another repo and this other one that are using the gettext-parser types and it the first one works flawlessly simply replacing the line "@types/gettext-parser": "^4.0.4"
, with "gettext-parser": "*"
In the second case as for the most of the users, however, it will suffice to remove the line "@types/gettext-parser": "^4.0.4"
since the repo already provides types, so the update will be very smooth
Hi @smhg, friendly ping on this since I haven't heard from you in a while, I modified the pr and also tested it on a couple of repos I was using it on and it works perfectly. If you have any doubts about anything tell me how to proceed
@smhg, I'm writing to provide an update regarding the pull request I opened. Since I haven't heard back from you in over a week, I've decided to proceed independently and have completely rewritten the module in TypeScript to adapt it to my needs.
Therefore, I would like to close the original pull request and publish my fork. If you have any questions or comments, please feel free to let me know otherwise i will proceed in this direction.
Hi @erikyo! Thank you for following up on this. I'm sorry I wasn't able to get back earlier.
I am not sure we are talking about the same things though. I meant to ask if you could create a new PR with the minor improvements your initial PR contained (typos, moving /lib to /src,...). Some of those I mentioned in the comments, but I basically mean everything unrelated to TS itself. I highly value those too + afterwards we can have a very clean TS PR (basically what remains in this PR). Can you have a look at that first? So please do send a new PR with all those unrelated changes.
Certainly! I was anticipating your approval of this PR before proceeding with additional small PRs to address the other minor inaccuracies (and at this point I think it is the best thing to do to avoid merge conflicts).
My plan is to submit one PR per issue, ensuring clarity and transparency throughout the process. I can create a tracking issue if you want (those with bullet points linking to the different pr) to wrap all the changes in a single issue. I apologize for any confusion, but this PR needed to implement significant changes to achieve the necessary improvements.
I think perhaps the best thing is to create a development
branch on which to merge the changes listed here until they are all completed. Then when finished you can rebase the development branch on master
What do you think? this way we can operate safely without modifying the main branch
This PR introduces TypeScript support and adds types within the npm package, addressing the suggestion raised in issue #80.
Changes include setting up the TypeScript environment, restructuring files for better organization, and adding strict types for module functions.
These enhancements aim to improve the module's compatibility with modern development practices and facilitate easier contributions from the community.
😇 Sorry for the 19 modified files but I really tried to keep it as small as possible!
close #80