2
\$\begingroup\$

Previously I asked a question to find the best practice of recursively running a function to retrieve all import/export statements of files in NodeJs here.

After some discussion I'm not satisfied yet, so I'm going to post my rewrote code from what I learned in previous discussion in hope of finding a better approach or simply getting review of my code.

I'm calling preprocessTSFiles function which runs getFiles with the entry-point and return all its import/export statements plus itself which leads to a recursive call of getImporters from VinylFile class.

preprocess-files.ts

import * as path from 'path';
import * as vinyl from 'vinyl';
import { VinylFile } from './file';
import {
  ensureMakeDir,
  isArray,
  isDirectory,
  isEmpty,
  isFile,
  isNil,
  readFile,
  writeFile,
} from './utils';

import { InlineStyles } from './plugins/inline-styles';
import { InlineTemplate } from './plugins/inline-template';

export interface HandlerPlugin {
  (file: VinylFile): Promise<string>;
}

export async function preprocessTSFiles(entryFilePath: string, destDir: string, baseDir?: string): Promise<void> {
  const fileList: string[] = [];
  const plugins = [InlineTemplate, InlineStyles];

  if (isNil(baseDir)) {
    baseDir = path.dirname(entryFilePath);
  }

  const allFiles = await getFiles(entryFilePath, []);

  for (const file of allFiles) {
    for (const plugin of plugins) {
      const newContent = await plugin(file);

      if (newContent != file.content) {
        file.setContent(newContent);
      }
    }

    const currentPath = file.filePath;
    const destPath = currentPath.replace(baseDir + path.sep, '');
    const absDestPath = path.resolve(destDir, destPath);

    file.copyTo(absDestPath);
  }
}

async function getFiles(entryFilePath: string, excludeList: string[]): Promise<VinylFile[]> {
  const entryFile = new VinylFile(entryFilePath);
  const excludeFromList: string[] = [entryFilePath];
  return [entryFile, ...await entryFile.getImporters(excludeFromList)];
}

file.ts

import * as path from 'path';
import {
  ensureMakeDir,
  isDirectory,
  isFile,
  isNil,
  readFile,
  writeFile,
} from './utils';

export class VinylFile {
  private _content: string;
  private importStatementRE = /(?:import|export)\s*(?:(?:\{[^}]*\}|\*|\w+)(?:\s*as\s+\w+)?(?:\s+from)?\s*)?([`'"])((?:\\[\s\S]|(?!\1)[^\\])*?)\1/ig;

  public dirPath: string;

  constructor(public filePath: string) {
    this.dirPath = path.dirname(filePath);
  }

  get content(): string {
    if (isNil(this._content)) {
      this._content = readFile(this.filePath);
    }

    return this._content;
  }

  public setContent(content: string): void {
    this._content = content;
  }

  public copyTo(destFilePath: string): void {
    const dirPath = path.dirname(destFilePath);
    ensureMakeDir(dirPath);
    writeFile(destFilePath, this.content);
  }

  public hasImporters(): boolean {
    this.importStatementRE.lastIndex = 0;
    return this.importStatementRE.test(this.content);
  }

  public async getImporters(excludeFrom?: string[]): Promise<VinylFile[]> {
    const importees: VinylFile[] = [];
    const importStatementMatchIndex = 2;
    let importersMatch: RegExpMatchArray | null;

    if (isNil(excludeFrom)) {
      excludeFrom = [];
    }

    // Reset current index to 0
    // RegEx.test moves this into 1
    this.importStatementRE.lastIndex = 0;
    while ((importersMatch = this.importStatementRE.exec(this.content)) && !isNil(importersMatch)) {
      const importee = importersMatch[importStatementMatchIndex];
      const resolvedPath = path.resolve(this.dirPath, importee);

      if (isDirectory(resolvedPath)) {
        const resolvedIndexFilePath = path.resolve(resolvedPath, 'index.ts');

        if (isFile(resolvedIndexFilePath) && excludeFrom.indexOf(resolvedIndexFilePath) < 0) {
          const resolvedIndexFile = new VinylFile(resolvedIndexFilePath);
          excludeFrom.push(resolvedIndexFilePath);
          importees.push(resolvedIndexFile);

          if (resolvedIndexFile.hasImporters()) {
            importees.push(...await resolvedIndexFile.getImporters(excludeFrom));
          }
        }
      } else {
        let resolvedFilePath = resolvedPath;
        if (path.extname(resolvedFilePath) !== '.ts') {
          resolvedFilePath += '.ts';
        }

        if (isFile(resolvedFilePath) && excludeFrom.indexOf(resolvedFilePath) < 0) {
          const resolvedFile = new VinylFile(resolvedFilePath);
          excludeFrom.push(resolvedFilePath);
          importees.push(resolvedFile);

          if (resolvedFile.hasImporters()) {
            importees.push(...await resolvedFile.getImporters(excludeFrom));
          }
        }
      }
    }
    return importees;
  }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I see your code is more readable after the refactoring.

You had choose better name that clarified the intention. This is a good part.

I think the thought point is the while part.

In a functional fashion you could improve the code putting the matching part in a method:

getImportStatements() {
  const importStatementMatchIndex = 2;
  const importStatements = [];
  let importersMatch = this.importStatementRE.exec(this.content);
  while(!isNil(impotersMatch)) {
    importStatements.push(impotersMatch[importStatementMatchIndex]);
    importersMatch = this.importStatementRE.exec(this.content);
  }
  return importStatements;
}

This will return just a list with the matching strings. The reason for this change is to try to split up the responsabilities in a way that your code will be much easy to read and change.

This method isolate the logic on how you get the list of imports on the source file.

Then you could rewrite the main method removing the while loop and adding the following:

this.getImportStatements()
  .map((importee) => (path.resolve(this.dirPath, importee)))
  .map((resolvedPath) => {
    if (isDirectory(resolvedPath)) {
      return path.resolve(resolvedPath, 'index.ts');
    }
    return (path.extname(resolvedPath) !== '.ts') ? resolvedPath += '.ts' : resolvedPath;
  })
  .filter((resolvedFilePath) => (!isFile(resolvedFilePath) || excludeFrom.indexOf(resolvedFilePath) >= 0))
  .forEach((resolvedFilePath) => {
    const resolvedFile = new VinylFile(resolvedFilePath);
    excludeFrom.push(resolvedFilePath);
    importees.push(resolvedFile);

    if (resolvedFile.hasImporters()) {
      importees.push(...await resolvedFile.getImporters(excludeFrom));
    }
  });

The principle is to attach on the array returned from getImportStatements(), that guarantee to return at least an empty array.

Then it use the javascript array methods to map the strings to full path, filter and then collect.

Fell free to change the arrow functions in methods to increase the readability of the code.

\$\endgroup\$
7
  • \$\begingroup\$ I really like what you did with js style of chain of command with arrays. \$\endgroup\$
    – PRAISER
    Aug 23, 2017 at 20:49
  • \$\begingroup\$ How do you merge return of getImportStatements with getImports? We need them all as an array in one place \$\endgroup\$
    – PRAISER
    Aug 23, 2017 at 22:05
  • \$\begingroup\$ @PRAISER you place the getImportStatements call inside the getImports removing the while loop. \$\endgroup\$ Aug 24, 2017 at 5:50
  • \$\begingroup\$ I'm talking about how should I put it, I need to return a promise in getImports but in getImportStatements we're not returning anything! \$\endgroup\$
    – PRAISER
    Aug 24, 2017 at 22:55
  • \$\begingroup\$ @PRAISER in getImportsStatements it returns an array, if you look at the code you see that in the last forEach statement, it accumulate the VinylFiles in the importees array, as was doing it in the while loop. So if you remove the loop and place the code in my answer, everithing should work as before. \$\endgroup\$ Aug 25, 2017 at 5:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.