Should we remove sync/async APIs, & Offer only a single API that returns a Promise?

[netroy]

This might help with avoiding issues like #132

[wtgtybhertgeghgtwtg]

While it'd be nice to remove the callbacks for a Promise API, I think taking out the synchronous API would be a bad move. If they already have the image in a Buffer, there's no reason to be asynchronous. What you could do to avoid the linked problem is to make the interface a bit more explicit

const imageSize = require('image-size');

// Always returns a Promise that'll get the size of the image from that file (or reject).
imageSize.fromFile('./myImage.png');

const myImageInABuffer = require('fs').readFileSync('./myImage.png');
// Always returns the size of the image from that buffer (or throws).
imageSize.fromBuffer(myImageInABuffer);

Alternatively, you could limit the scope of the project to just the synchronous method and make getting the Buffer the responsibility of the user, as they currently have to do with images taken over HTTP.

const imageSize = require('image-size');

getImageFromDisk().then(buffer => imageSize(buffer));
getImageFromInternet().then(buffer => imageSize(buffer));
getImageFromCanvas().then(buffer => imageSize(buffer));

[AMorgaut]

Moving to v1.0 is clearly a big step :-)

As it is a major version, breaking some legacy API compatibility is technically allowed by semver but does it worth giving pain in the a... ?

I would not remove the sync version for now

Wide usage of it is also for writing CLI tools for which it is often handy (even if not optimized) to be able to call the APIs synchronously (some grunt plugins are sync code dependent).

At least I would either:

• wait for an image-size version that only supports node versions supporting async/await as this makes the async code as easy to write than sync code, and the migration note it would require should be way more simple to apply.
• or provide an alternate npm module for the sync version
• in any case, some previous 0.x image-size versions should then start to deprecate the sync API with util.deprecate()

Regarding the async API

It is very easy & light to support both the callback & promise APIs. The only issue is if we want to support it with the current API signatures.

The polymorphic approach cannot choose to treat sizeOf('images/funny-cats.png') callback less call as a promise expected version as it is already used for the sync version...

When I first started to look at image-size I was a bit troubled by it directly returning the function.
I do think that the very most clean approach for v1.0 would be to expose the main variants through dedicated API names.

This could give:

const imgSize = require('image-size');

with

• main API:
  • imgSize.async(imgPath) => async call returning a promise (async/await compatible)
  • imgSize.sync(imgPath) => explicit sync call. Projects reviewers are more aware of implications
• retro API
  • imgSize(imgPath) => sync call with deprecation warning "Use the sync() method instead"
  • imgSize(imgPath, callback) => legitimate callback async call. No deprecation (at least for v1).

Advantages

• No breaking changes / Backward compatible
  • const sizeOf = require('image-size'); would still work as is
• Usage is more explicit
• Usage can be as simple as before with still possibility to require directly the expected API
  • ex1: const mySizeOfName = require('image-size').async;
  • ex2 with more statically set sizeOf/sizeOfSync method names:
    • const { sizeOf } = require('image-size');
    • const { sizeOfSync } = require('image-size');
    • const mySizeOfName = require('image-size').sizeOf;
    • const { sizeOf: mySizeOfName } = require('image-size');

[jimmywarting]

I like the idea of a Onion Architecture

The idea should be that it should have as little knowledge about a system environment as possible.
In the core it should not have any way of reading a file from the filesystem or the network - it should also not either use any specific NodeJS, Deno or Browser specifics modules/api's.
It should only be a synchronous api layer around ArrayBuffer, Uint8Arrays, and DataView (it should not use node's Buffer). Then this package/layer can better fit into Node, Deno and the browser without bundle in many unnecessary platform specific codes

Deno has a security layer where 3th party modules are sandboxed and have no way to access to the File IO, or the network
It feels much more safer to just hand 3th party packages the data then giving it full access/control. Piping data is way better

Then the next layer should be platform specifics, Reading files from the fileSystem, urls, sockets, or whatever. This could be extracted out as a separate file, But overall i think it's better if just the users/developers deals with this instead

// Where a web developer could simply do:
const {width, height} = await blob.slice(0, 1024).arrayBuffer().then(sizeOf)

// cross platform way of doing stuff
const {width, height} = sizeOf(arrayBuffer)

You can also have a utility file that isn't imported directly. Developers would have to import it manually

import { sizeFromFs } from 'image-size/from-node-path.js'