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

Inconsistent/hard to use API: consider new major? #142

Open
fatso83 opened this issue Feb 7, 2018 · 2 comments
Open

Inconsistent/hard to use API: consider new major? #142

fatso83 opened this issue Feb 7, 2018 · 2 comments

Comments

@fatso83
Copy link

fatso83 commented Feb 7, 2018

The current API is quite awkward and doesn't conform to any modern conventions/standards. It's hard to know when anything goes wrong, as it doesn't pass or throw any errors, and it evens opts to return false synchronously from async methods. There is of course no way of detecting errors in the async parts using this approach.

Another nuisance is the way some API methods expect to be used within a context created by another API method. This is so weird and makes casual exploration of the library impossible (as you don't signal wrong use either). EXIF.getData( function(){ const foo = EXIF.readAllTags(this); // WTF? }). The implicit using of the context also makes using arrow functions impossible.

All in all, the library has a lot of good functionality and code, but the packaging is somewhat lacking. Would you consider a PR that simplified the API into a more modern package that would support a more conventional and modern API? The async methods should employ Promises or Node-style callbacks, and should consistently return an object you could ask for information on (the context object in the old API).

Something like

const imgNode = document.getElementById('myImgTag');

EXIF.fromImage(imgNode, (err, exifInstance) => {
   if(err) return handleError(err);
    doFoo(exifInstance.allTags());
});

// or using a promise api
EXIF.fromImage(imgNode)
  .then(exifInstance) => exifInstance.allTags())
  .then(dooFoo)
  .catch(handleError);
});

It would of course not be backwards compatible, without an additional exif-js-v2-wrapper, but I doubt many will miss the old API 😸

@Ankcorn
Copy link

Ankcorn commented Mar 21, 2018

I agree, exploring this library was offputting but it works so well. So much hard work has gone into this project and it works so well but I fear people will not use it or contribute because of the high barrier to entry.

I would be happy to help out simplifying the code base.

A starting point would be to write some tests for some of the lower level functionality to ensure people can contribute without breaking core functionality. I propose Ava.js as a test library. It's lightweight and very fast.

Do any of the project's contributors to weight in?

@fatso83
Copy link
Author

fatso83 commented Mar 22, 2018

I guess this project is a bit on the backburner, so my suggestion is to push forward:

  • fork the project
  • make changes to the API until you feel something is coming along nicely
  • report back here when you want people to test it out and provide feedback (remember they should only need to do npm install ThomasAnkcorn/exif-js to install it directly from your GitHub master branch if you set up the project correctly)
  • once the kinks have ironed out I would then supply a PR

If the PR just ends up rotting without progress, just publish an NPM artifact under your own NPM namespace npm install @thomasankorn/exif-js as a reasonable compromise.

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

2 participants