opensearch-project / OpenSearch-Dashboards

📊 Open source visualization dashboards for OpenSearch.
https://opensearch.org/docs/latest/dashboards/index/
Apache License 2.0
1.67k stars 876 forks source link

Replace `util.promisify` with direct promise based functions #3919

Open ananzh opened 1 year ago

ananzh commented 1 year ago

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 https://github.com/opensearch-project/OpenSearch-Dashboards/issues/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 commented 1 year ago

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 commented 1 year ago

Promisify on ncp

Sample usage

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

How to remove promisify

ananzh commented 1 year ago

Promisify on glob

Sample Usage

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

How to remove promisify

import glob from 'glob-promise';
ananzh commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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 importpipeline 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';