Don't add parameters after functions

[fregante]

While this reads fine:

const addOneListener = memoize(addListener, {
	cacheKey: arguments_ => arguments_,
	cache: new ManyKeysMap()
});

This is bad practice:

const addOneListener = memoize(() => {
	// some


	// inline


	// long


	// function


	// which


	// is not


	// uncommon
	return stuff
}), {
	cacheKey: arguments_ => arguments_,
	cache: new ManyKeysMap()
});

This could be enforced by a linter, since APIs like addEventListener(string, cb, options) exist natively, but ideally memoize and p-memoize should accept a single parameter:

function memoize(details: Callback | Options) {
	const {callback, ...options} = typeof details === 'function' ? {callback: details} : details;
}

It complicates types a bit but it requires the much cleaner:

const addOneListener = memoize({
	cacheKey: arguments_ => arguments_,
	cache: new ManyKeysMap(),
	callback: () => {
		// some
	
	
		// inline
	
	
		// long
	
	
		// function
	
	
		// which
	
	
		// is not
	
	
		// uncommon
		return stuff
	}),
});

---

Personal note: I went through this recently in webext-storage-cache, changing (fn, opts) => {} to (fnOrOpts) => {}

[sindresorhus]

I don't see why the API has to change. Just not put the function inline.

[sindresorhus]

but ideally memoize and p-memoize should accept a single parameter:

I disagree about that being ideal. I find that API quite weird. There are now two ways to do the same thing.

[fregante]

Any API that allows unreadable code is not good for me, similarly to why functions should not have 5 parameters.

Inline functions are nice but they grow and people don't move them out. Should I open an issue on unicorn? 😃

[sindresorhus]

Should I open an issue on unicorn?

👍

[fregante]

I find that API quite weird. There are now two ways to do the same thing.

have you seen fetch 🙈

fetch('flowers.jpg', {method: 'POST'})

const myRequest = new Request("flowers.jpg", {method: 'POST'});
fetch(myRequest)

two ways to do the same thing.

I mean, that's pretty common, but I don’t think anyone would prefer mem({cb: () => {}}) over mem(() => {}) unless the API requires it (for good reasons, as shown here)

[fisker]

Maybe we can change signature to

memoize(options?, () => {})

?

options can't be a function anyway.

[fregante]

That sounds almost good but I think (options, callback) is not a common pattern

[fisker]

(Callback | Options) is not diff friendly

memoize({
-   cb() {
-     // .... indention level 2
-   },
-   cache: new ManyKeysMap,
})

memoize(() => {
+   // .... indention level 1
})

[meduzen]

Inline functions are nice but they grow and people don't move them out.

They do when you tell them. :p

[fregante]

Inline functions are nice but they grow and people don't move them out.

They do when you tell them. :p

Sounds like a linter job, but that rule is complex and I think it's not in the recommended config, so it's easier to just avoid bad patterns at the root

[fregante]

(Callback | Options) is not diff friendly

Lots of things are not diff friendly, like Prettier, and yet… they're worth it.