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

Should sepa.js be rewritten to Typescript? #70

Open
fellmann opened this issue Jul 9, 2024 · 10 comments
Open

Should sepa.js be rewritten to Typescript? #70

fellmann opened this issue Jul 9, 2024 · 10 comments

Comments

@fellmann
Copy link
Contributor

fellmann commented Jul 9, 2024

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

kewisch commented Jul 9, 2024

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

leMaik commented Jul 22, 2024

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

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:

  • Direct Debit export
  • Transfer export
  • Statement import (partially)
  • Feature flags for different file versions
  • Modern build tooling
    Still missing a few test cases being rewritten, readme, examples etc.

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

@kewisch
Copy link
Owner

kewisch commented Jul 23, 2024

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

leMaik commented Jul 24, 2024

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. 💪

@kewisch
Copy link
Owner

kewisch commented Jul 24, 2024

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

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

  • I believe this library is almosts exclusively used in the backend as of today.
  • Even if it is used in the frontend, a bundler like webpack is used, and browser compatibility is ensured by transpiling the code.
  • In the backend, there is no native XML library available, so an external library like xmldom is always needed. Thus, it is better to use a modern and easy to use library instead of the cumbersome and old XMLSerializer.
  • The risk of introducing bugs within the library due to the complicated XML generation is much higher than bugs from using an external XML library.
  • The highest risk for bugs results from using plain JavaScript instead of a type-safe language.

@kewisch
Copy link
Owner

kewisch commented Sep 11, 2024

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.

@kewisch kewisch changed the title Rewrite with TypeScript Should sepa.js be rewritten to Typescript? Sep 11, 2024
@kewisch kewisch pinned this issue Sep 11, 2024
@leMaik
Copy link

leMaik commented Sep 27, 2024

@kewisch As noted before, refactoring the code and adding jsdoc would probably be the same effort as rewriting it with TypeScript. Now that type declarations have been merged, i see even less reasons to add jsdoc.

@kewisch
Copy link
Owner

kewisch commented Jan 5, 2025

Happy new year! I'd be ok trying out the typescript approach for this library. New year, ready to try some new things :)

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

No branches or pull requests

3 participants