kewisch / sepa.js

Create and validate SEPA XML transactions
http://kewisch.github.io/sepa.js
Other
85 stars 61 forks source link

Should sepa.js be rewritten to Typescript? #70

Open fellmann opened 2 months ago

fellmann commented 2 months ago

Because I need this for my project, I started to rewrite this library using TypeScript, a different XML builder and modern tooling. @kewisch Are you open to include this as "v2" in this repository?

EDIT from @kewisch: If you are reading this and using sepa.js in your code, could you put a thumbs up/down on if you think we should convert this to typescript instead? This will help inform a path forward.

kewisch commented 2 months ago

I'm not a big typescript fan. If you want the best of both worlds then we could do what we did for https://github.com/kewisch/ical.js , use jsdoc to generate the types. Would you be open to adapting that approach?

I think for this library my only other requirement is reducing dependencies that are not devDependencies. Since money is involved I'd rather not spend too much time auditing dependencies. If needed we could pin them and update with a code review.

I'm all up for modern tooling though, I almost started to make some updates recently because it was annoying me that the tools were so old/basic.

leMaik commented 1 month ago

I looked into the approach that @kewisch suggested and managed to add some jsdoc (as a poc). However, generating the .d.ts files uses TypeScript via rollup in ical.js, which in turn requires es modules.

At that point, it would imho be less effort to ~rewrite~ migrate sepa.js to TypeScript (with the additional benefit of type safety) and then use tsdoc to generate docs than to update the code to es modules, jsdoc and type definition generation. The two custom jsdoc plugins also have their own overhead.

fellmann commented 1 month ago

Sorry for my delayed answer. My (opionated) approach is not to write any new code in plain JavaScript. Compared to TypeScript, I see a lot of downsides and almost no advantage. Plus, when working with the code, I felt the approach a bit complicated and hard to extend, while the design of instantiating classes and setting properties subsequently does not play well with type-safety. (But, the concept was absolutely fine a few when this library was written.) In the meantime, I have started a rewrite using TypeScript and fast-xml-parser. It supports:

Feel free to have a look at the current state and comment: https://github.com/fellmann/sepa.js/tree/rewrite

kewisch commented 1 month ago

I agree with you that it all comes down to opinion and preference, there is probably no right answer. There are many advantages to type safety in general. What I don't like about Typescript is the need for a transpiling/compiling step. I'd prefer to stay as close to the engine as possible, which makes debugging easier, avoids the runtime overhead, and keeps us more platform agnostic. It appears that the jsdoc approach plays well with IDEs doing type checking, so we'd get the best of both worlds.

The two custom jsdoc modules were a bit of a hack to get the docs to look like I expected (I wanted it to say e.g. SEPA.PaymentInfo instead of just PaymentInfo, while making it tsc compliant required just the class name). It doesn't appear there are any issues with type safety when using a toplevel object like SEPA, but I might just not have discovered it yet.

If it helps, I'd be willing to skip using a toplevel SEPA object, which would alleviate the need for those custom jsdoc plugins. Would you be willing to make compromises to maintain a new version using js + jsdoc? I'm happy to help migrate some of the tooling.

leMaik commented 1 month ago

I started migrating to js + jsdoc as a proof of concept. Maybe we should switch to es modules (similar to ical.js) while we're at it? I'm happy to help with the migration, but I'd feel uncomfortable with the idea of maintaining a (typescript or whatever) fork. It's nice to have people collaborating here to conform to the latest standards. :muscle:

kewisch commented 1 month ago

I agree, I'd love to continue working with you on this. I'm +1 on ES modules, there is sufficient support in browsers and node for this to be standard.

fellmann commented 1 month ago

Let me add some more arguments for my point of view:

kewisch commented 4 days ago

I'm seeing more and more people mention typescript in this repo. I think it all comes down to who is willing to stay with me on this journey. If those most likely to contribute patches feel more comfortable with typescript, then let's take that path. If this is more temporary and I'll be doing most of the maintaining, then I'd rather not make the jump to TS.

@leMaik How is your proof of concept going?

If folks reading this issue and making use of sepa.js could simply put a thumbs up or down on the initial issue, it might help give a measure on where the crowd stands.