What is not right in this code? Post your solution.

const fs = require("fs");
const { promisify } = require("util");

const cache = new Map();

cache.set("file1", "data of file 1...");
cache.set("file2", "data of file 2...");

const readFilePromise = promisify(fs.readFile);

// What is wrong in this function?

const getFileData = (fileName, callback) => {
    if (cache.has(fileName)) return callback(null, cache.get(fileName));
    return readFilePromise(fileName)
        .then(data => {
            cache.set(fileName, data);
            return callback(null, data);
        }).catch(err => callback(err));
};

Comments

[J. Pichardo]

Well besides this line ...

cache.set("file2, 'data of file 2...");

// Should be (Note: quotes)

cache.set("file2", "data of file 2...");

This are the changes I'd make

1. Use object instead of Map (Easier to have immutable structures)
2. Wrap cache in a Promise.resolve in order to reuse code and be able to use a ternary operator
3. For consistency DO NOT use a callback syntax for a promise based function

So, the final script would be:

const cache = {
  ["file1"]: "data of file 1...",
  ["file2"]: "data of file 2..."
};

const getFile = fileName => {

  // Wrap the cache in a promise in order to do chaining;
  const cachePromise =
    cache[filename] != null
      ? Promise.resolve(cache)
      : readFilePromise(fileName).then(
          content => (cache = { ...cache, [fileName]: content })
        );

  // Return a promise with the data
  return cachePromise.then(cache => cache[fileName]);
};

// DO NOT pass a callback.
getFile("file1")
  .then(data => {
    // Do Something
  })
  .catch(err => {
    // Do Something else
  });

[Ben Sinclair]

Well it took me > 10 second to twig that "cb" meant "callBack" so there's that: use explicit variable names. Same with err - is it a message or a code or an object or what?

Other than that I have no idea what the braces around { promisify } are for because I don't do this language and am not sure what to google for to find out. I don't know any of these libraries so don't know what to expect.

Having said that, shots in the dark from an outside developer:

• there's nothing that sets an expiry on the cached data - if I assume cache.has returns true even if the data's expired, and that you are expected to check some expiry time property, then it will never get updated.
• fs.readFile might process a condition which is transiently invalid without throwing an exception, in which case it'll be stored for eternity.
• cache.get(..) might return an object containing metadata alongside the actual payload, such as expiry time

[Leighton Darkins]

To address your intrigue about the braces around promisify, this article may shed some light:

Object Destructuring in ES6

Sarah Chima

#es6#javascript

[Ben Sinclair]

Oh that's cool. Just tried it in my node REPL and found whatever version of node you get with MacOS is too old (no surprises there though)