electron / release-status

Public facing release status information
https://releases.electronjs.org
MIT License
3 stars 8 forks source link

Incorrect use of `cacheKey` in `data.js` #50

Closed piotrpdev closed 4 months ago

piotrpdev commented 4 months ago

@dsanders11

cacheKey is of this type:

https://github.com/sindresorhus/p-memoize/blob/aa01febcd1104b2aad7e5fb05040befc8493daf1/index.ts#L87 https://github.com/sindresorhus/p-memoize/blob/aa01febcd1104b2aad7e5fb05040befc8493daf1/index.ts#L46

{
    readonly cacheKey?: (arguments_: Parameters<FunctionToMemoize>) => CacheKeyType;
}

where arguments_ is an array. This is what typical use looks like:

https://github.com/sindresorhus/p-memoize/blob/aa01febcd1104b2aad7e5fb05040befc8493daf1/test.ts#L108

const memoized = pMemoize(fixture, {cacheKey: ([firstArgument]) => String(firstArgument)});

Therefore, these lines are not carrying out their intended behaviour:

https://github.com/electron/release-status/blob/6718e2627b614fca0bc96f48f9778ba45de8f9ba/src/data.js#L90 https://github.com/electron/release-status/blob/6718e2627b614fca0bc96f48f9778ba45de8f9ba/src/data.js#L115 https://github.com/electron/release-status/blob/6718e2627b614fca0bc96f48f9778ba45de8f9ba/src/data.js#L137 https://github.com/electron/release-status/blob/6718e2627b614fca0bc96f48f9778ba45de8f9ba/src/data.js#L155 https://github.com/electron/release-status/blob/6718e2627b614fca0bc96f48f9778ba45de8f9ba/src/data.js#L166

dsanders11 commented 4 months ago

Nice catch @piotrpdev!

I think functionally they're still having their desired effect since Array.prototype.toString() stringifies to a comma separated list of values, so in the case where there's only one argument it is producing the same string output as it would if implemented correctly.

Even in the two argument case (line 155) it still produces a cache key that includes all the necessary information, just not in the expected format: compare/v32.0.0-nightly.20240418,67ba30402bcbf6942fa0846927f2d088cf0a749d/undefined.

That said, you're correct that the usage is wrong. Happy to take a PR which corrects the argument types to be arrays. 👍