Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace util.promisify with direct promise based functions #3919

Open
ananzh opened this issue Apr 21, 2023 · 6 comments
Open

Replace util.promisify with direct promise based functions #3919

ananzh opened this issue Apr 21, 2023 · 6 comments
Assignees
Labels
technical debt If not paid, jeapardizes long-term success and maintainability of the repository.

Comments

@ananzh
Copy link
Member

ananzh commented Apr 21, 2023

Starting from Node.js 8 onwards, most of the core APIs in Node.js have been updated to support native Promises, making the use of util.promisify less relevant for newer versions of Node.js. util.promisify was initially introduced to convert callback-based APIs to Promise-based APIs, making it easier to work with asynchronous code using async/await. With the addition of native Promise support in core APIs, there is no longer a need to use promisify for these functions, as you can now directly use the Promise-based versions of them.

As part of Node.js 18 bump work #3601, we decide to remove all the promisify usages in the repo. This is a research and document task to record all the changes.

@ananzh ananzh added the enhancement New feature or request label Apr 21, 2023
@ananzh ananzh added NODE 18 ⚙ v2.8.0 and removed enhancement New feature or request untriaged labels Apr 21, 2023
@ananzh ananzh self-assigned this Apr 21, 2023
@ananzh
Copy link
Member Author

ananzh commented Apr 21, 2023

Promisify on cmdShimCb

Sample usage

import cmdShimCb from 'cmd-shim';
import { promisify } from 'util';

const cmdShim = promisify<string, string>(cmdShimCb);

How to remove promisify

cmd-shim doesn't have a native Promise-based version, but latest cmd-shim already use promise. So the fix is to bump current cmd-shim version.

@ananzh
Copy link
Member Author

ananzh commented Apr 21, 2023

Promisify on ncp

Sample usage

import { ncp } from 'ncp';
export const copyDirectory = promisify(ncp);

How to remove promisify

  • Use ncp-promise.
import ncp from 'ncp-promise';

@ananzh
Copy link
Member Author

ananzh commented Apr 21, 2023

Promisify on glob

Sample Usage

import { glob } from 'glob';
export const globAsync = promisify(glob);

How to remove promisify

  • Replace glob to glob-promise where glob function returns a promise. In package.json, we should keep @types/glob. The reason is that glob-promise is a wrapper around the glob library, and the type definitions for the original glob package are provided by @types/glob. The glob-promise package itself does not have its own type definitions, but it uses the types from glob for the most part.
import glob from 'glob-promise';

@ananzh
Copy link
Member Author

ananzh commented Apr 21, 2023

Promisify on fs functions that have a promise version in fs.promises

The solution is to import directly and use functions in fs.promises

fs.stat

Fs.stat usage

import Fs from 'fs';
import { promisify } from 'util';
const statAsync = promisify(Fs.stat);

should be changed to

import { stat } from 'fs/promises';
const stats = await stat(osd.getAbsolute(path));

fs.mkdir

@ananzh
Copy link
Member Author

ananzh commented Apr 21, 2023

Promisify on fs.exists

Usage example

import { exists } from 'fs';
import { promisify } from 'util';

const existsAsync = promisify(exists);

How to remove promisify

The fs.promises.exists method doesn't exist, as the exists function was deprecated in favor of access. We should use fs.promises.access directly to check for the existence of a file. There are two use cases:

  • If don’t need to check permissions, could use the following snippet
import { access } from 'fs/promises';

const exists = async (path) => {
  try {
    await access(path);
    return true;
  } catch (e) {
    if (e.code !== 'ENOENT') throw e; // If the error is not 'ENOENT', rethrow the error
    return false; // If the error is 'ENOENT', it means the file does not exist, so return false
  }
};
  • If need to check permissions, for example check if a file exists and if it has write permissions to delete the file, could use the following example which compared before and after using access as a reference.
#before
import del from 'del';
import fs from 'fs';

export function cleanPrevious(settings, logger) {
  return new Promise(function (resolve, reject) {
    try {
      fs.statSync(settings.workingPath);

      logger.log('Found previous install attempt. Deleting...');
      try {
        del.sync(settings.workingPath, { force: true });
      } catch (e) {
        reject(e);
      }
      resolve();
    } catch (e) {
      if (e.code !== 'ENOENT') reject(e);

      resolve();
    }
  });
}

export function cleanArtifacts(settings) {
  // delete the working directory.
  // At this point we're bailing, so swallow any errors on delete.
  try {
    del.sync(settings.workingPath);
  } catch (e) {} // eslint-disable-line no-empty
}
#after
import { rm, access } from 'fs/promises';
import { rmSync, constants } from 'fs';

const exists = async (loc) => {
  try {
    await access(loc, constants.W_OK);
    return true;
  } catch (e) {
    if (e.code !== 'ENOENT') throw e;
  }
};
export const cleanPrevious = async (settings, logger) => {
  const workingPathExists = await exists(settings.workingPath);

  if (workingPathExists) {
    logger.log('Found previous install attempt. Deleting...');
    return await rm(settings.workingPath, { recursive: true });
  }
};

export function cleanArtifacts(settings) {
  // Delete the working directory; at this point we're bailing, so swallow any errors on delete.
  try {
    rmSync(settings.workingPath, { recursive: true, force: true });
  } catch (e) {} // eslint-disable-line no-empty
}

@ananzh
Copy link
Member Author

ananzh commented Apr 21, 2023

Promisify on stream

Starting from Node.js v15, the stream/promises API provides an alternative set of asynchronous utility functions
for streams that return Promise objects rather than using callbacks. The API is accessible via import {...} from 'stream/promises' or require('stream').promises.

pipeline

We can import pipeline directly from stream/promises and remove promise function. Here is an example:

# before
import { pipeline } from 'stream';
const asyncPipeline = promisify(pipeline);
# after
import { pipeline } from 'stream/promises';

@ananzh ananzh added v2.9.0 technical debt If not paid, jeapardizes long-term success and maintainability of the repository. and removed NODE 18 ⚙ v2.8.0 labels May 22, 2023
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this issue May 23, 2023
Issue Resolve
opensearch-project#3919

Signed-off-by: ananzh <ananzh@amazon.com>
@ananzh ananzh added v2.10.0 and removed v2.9.0 labels Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical debt If not paid, jeapardizes long-term success and maintainability of the repository.
Projects
None yet
Development

No branches or pull requests

2 participants