replace mem dependency with fast-memoize (smaller, faster) #671
Conversation
There was a problem hiding this comment.
Thanks for the contribution, my apologies for the delay. I approved the CI run, sorry I missed it before.
Unfortunately the mem/memoized cache is something I really do not understand at this point, so any resources that can help me better understand this improvement would be extremely helpful. I did notice you proposed a contribution to nano-memoize, don't know if that might give us even better improvement or not.
I think it would also be ideal if you could propose this enhancement to the main Prettier project first, where I think it might get some more expert feedback. In case this improvement would not be accepted by the main Prettier project, I would still be happy to consider it in prettierX with their feedback in mind.
In general, I think it would be ideal if we could find a way to add a module such as src/common/memoize. Or maybe if we could see if some of the duplicated code could be moved into its own, new npm package. You may see that I subscribe to this philosophy: https://blog.sindresorhus.com/small-focused-modules-9238d977a92a
| const memoizeOptions = { | ||
| cache: { | ||
| create() { | ||
| return { | ||
| has: (key) => memoizeCache.has(key), | ||
| get: (key) => memoizeCache.get(key), | ||
| set: (key, value) => memoizeCache.set(key, value) | ||
| }; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
This looks like duplicated code to me
| const memoizeOptions = { | ||
| cache: { | ||
| create() { | ||
| return { | ||
| has: (key) => memoizeCache.has(key), | ||
| get: (key) => memoizeCache.get(key), | ||
| set: (key, value) => memoizeCache.set(key, value) | ||
| }; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
looks like duplicated code to me
| const memoizedLoad = memoize(load, memoizeOptions); | ||
| const memoizedSearch = memoize(findPluginsInNodeModules, memoizeOptions); |
There was a problem hiding this comment.
There are lint:typecheck errors in these lines but I would be happy to help resolve these once we get my other comments and questions resolved.
| const memoizeOptions = { | ||
| cache: { | ||
| create() { | ||
| return { | ||
| has: (key) => memoizeCache.has(key), | ||
| get: (key) => memoizeCache.get(key), | ||
| set: (key, value) => memoizeCache.set(key, value) | ||
| }; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Looks like duplicated code to me
| } | ||
| } | ||
| }; | ||
| const jsonStringifyMem = (fn) => memoize(fn, memoizeOptions); |
There was a problem hiding this comment.
I would be happy to help get the lint:typecheck error resolved here once we get my other comments and questions resolved.
|
no worries, i'll try get it sorted in prettier's repo too. really its just trying to cache the plugins list/loading between multiple calls. honestly i doubt a dependency is even needed in reality, prettier should have kept the plugins in a cache/context themselves and only called the function to populate it once IMO. instead of relying on caching a function's result. anyway ill go sort it out in prettier and update this afterwards |
I see prettier/prettier#11230, thanks! One comment I have from description in prettier/prettier#11230:
It makes me start to wonder if it would make sense to consider using macros ... see brody4hire/ask-me-anything#35 |
|
OK it looks like the upstream work moved into prettier/prettier#11241 and has not moved forward as smoothly as I had hoped and expected. I probably should have explained more clearly that upstream Prettier uses their build step to publish with all dependencies included. I think it would be helpful for both upstream Prettier and this fork if we could get a better understanding of the motivation behind cleaning up this one dependency for the caching. My impression is that the "bloat" of the existing More important to me (at least) is ease of understanding. I am very sorry to say that I have not found any of the proposals to be very easy to understand. I think it would be ideal if we could find a way to make the cached config mechanisms both less bloated and easier to understand at the same time. Another topic which I think belongs in a separate discussion is the effect of publishing to npm directly vs publishing from a build. Publishing from a build like Prettier does makes installation a little easier, seems to fix ESM support, and supports the browser more directly. Disadvantages would include need for explicit license statement for bundled dependencies and hiding of vulnerabilities such as possible REGEX-DOS in bundled dependencies. |
the mem dependency pulls in a few levels deep worth of dependencies for something extremely simple.
here i've replaced it with fast-memoize which has no dependencies and seems much faster/simpler.
if the
memoizeOptionsrepetition puts you off, we can probably just change it tocreate() { return memoizeCache; }since a map coincidentally follows the same interface fast-memoize expects.let me know if there's any changes you want, feel free to let me know if you're not interested in this change too