tldr-node-client icon indicating copy to clipboard operation
tldr-node-client copied to clipboard

Bump minimum engine version to 8+

Open MasterOdin opened this issue 4 years ago • 1 comments

The current minimum engine version is Node 6 for this client. That version of node was EOL in April 2019. The next LTS version (8) was EOL in December 2019 and 10.x was EOL in April 2021.

While we could jump to 10+, minimally, it would be nice to move to 8+ as it would allow taking advantage of async/await for promises and greatly clean-up some of the nested promise chains the project heavily uses.

For example:

Current code:

  printBestPage(command, options={}) {
    // Trying to get the page from cache first
    return this.cache.getPage(command)
      .then((content) => {
        // If found in first try, render it
        if (!content) {
          // If not found, try to update
          return spinningPromise('Page not found. Updating cache...', () => {
            return this.cache.update();
          })
            .then(() => {
              return spinningPromise('Creating index...', () => {
                return search.createIndex();
              });
            })
            .then(() => {
              // And then, try to check in cache again
              return this.cache.getPage(command);
            });
        }
        return content;
      })
      .then((content) => {
        if (!content) {
          throw new MissingPageError(this.config.pagesRepository);
        }
        return this.checkStale()
          .then(() => {
            this.renderContent(content, options);
          });
      });
  }

async/await version:

  async printBestPage(command, options={}) {
    // Trying to get the page from cache first
    let content = await this.cache.getPage(command);
    if (!content) {
      await spinningPromise('Page not found. Updating cache...', () => {
        return this.cache.update();
      });
      await spinningPromise('Creating index...', () => {
        return search.createIndex();
      });
      content = await this.cache.getPage(command);
    }
    if (!content) {
      throw new MissingPageError(this.config.pagesRepository);
    }
    await this.checkStale();
    return this.renderContent(content, options);
  }

14 lines shorter and no nesting, and exact same functionality (I think, I eyeballed this).

There are a number of places that would benefit from this. The downside would be breaking compatibility with old versions of node, but given how EOL it's been, it should be fine, and of course old client versions would still work.

MasterOdin avatar Nov 01 '21 20:11 MasterOdin

@MasterOdin @navarroaxel - If you folks would be up for revamping the codebase to the modern async/await syntax and support latest node LTS versions, feel free to go ahead. We'd just need to bump up the major version to 4. It should be fine as we haven't had a major release in a long time.

Also related is: https://github.com/tldr-pages/tldr-node-client/pull/352

agnivade avatar Sep 04 '22 07:09 agnivade