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

Make core available as standalone consumable modules #42

Conversation

alloy
Copy link

@alloy alloy commented Jan 27, 2021

Opening this early and unpolished to illustrate what it could look like and to discuss requirements. I tried to stay as close as possible to your original intent/style, but can totally understand if this is too much of a change for you. (I can always just use this forked code myself, as I don’t imagine there will be much changes in the future.)

Changes:

  • Changes to ES6 modules loadable as-is by modern browsers; which means no IE support out-of-the-box. The reason for this is that some form of exporting was necessary to consume these modules elsewhere. (CommonJS exports would always require some extra step/tooling to support browsers.)
  • Adds some more JSDoc to allow the use of the TypeScript compiler to provide some type-checking and IDE features.
  • Replaced minified version of notie with original ES6 version.
  • Initial refactoring to move browser specific code out into its own module.

Considerations:

  • Transpile to bundle that supports IE? Requires build-time tooling.
  • Perhaps rename files as .mjs to signal they are ES6 modules?
  • Perhaps change to use ES6 classes, which mostly means being able to have the TS compiler infer more with less docs needed (I think).
  • Need to figure out the best way to provide input data, as the HTML5 File API does not exist natively in NodeJS (nor does any polyfill package seem to provide all the relied upon APIs).

Closes #41

@@ -196,6 +196,9 @@ const CSVGood = function (file, onStep, onError, onComplete) {

fileReader.onload = (evt) => {
// Take result
if (!evt.target) {
throw new Error("Expected event to have a target")
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added all these types of checks to satisfy the type-checker errors thrown by the TS compiler where technically APIs can return null/undefined.

}
});
}
};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically a collection of all browser based code I collected from convert.js

@DanielHaitink
Copy link
Owner

Hi, Sorry for the late reply. Have been a bit busy.

Although I am very glad you put so much effort into this, I think these changes are too big for me personally. Especially since we are talking about a small feature. If I were to create an electron build of the system, I think I would be okay with this. However I believe the system is easier and just as safe to use in the browser browser.

You are of course free to fork this project and change all the code as needed.

The only changes that I can foresee (except bugfixes and added support for banks which are easily integrated into your fork) are a merge with the ABN AMRO version of YNABGoingDutch #25 and an integration into the YNAB api #26 . However these features are not guaranteed to be implemented ever.

I can always link to your repo in the readme.md if you'd like. Thanks again for the effort you put into this!

@alloy
Copy link
Author

alloy commented Feb 1, 2021

Makes total sense and no worries, I need this so there’s nothing lost here.

The only changes that I can foresee (except bugfixes and added support for banks which are easily integrated into your fork) are a merge with the ABN AMRO version of YNABGoingDutch #25 and an integration into the YNAB api #26 . However these features are not guaranteed to be implemented ever.

I’m tracking the API one, but also don’t foresee it being a real problem to merge back in if it ever gets done.

I can always link to your repo in the readme.md if you'd like.

Yeah that would be nice, I’m sure there will be some other dev at some point that wants this 👍

Thanks again for the effort you put into this!

Thank you for the original work!

@alloy alloy closed this Feb 1, 2021
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.

Publish core conversion code as npm package
2 participants