jslicense / licensee.js

check dependency licenses against rules
https://www.npmjs.com/package/licensee
Apache License 2.0
185 stars 23 forks source link

`filterPackages` callback option #63

Closed brettz9 closed 4 years ago

brettz9 commented 4 years ago

This fixes #53 .

Rather than adding the complexity of directly supporting filtering for specific packages and their children, this adds a simple callback option by which the user can do so. It is an intermediate solution for @kemitchell 's helpful suggestion. It avoids the user having to run readFilesystemTree separately on their own.

(In case anyone was wondering why I did not express as packages = packages.filter(configuration.filterPackages), this per-item filtering would not be sufficient, as the filtering needed for determining dependencies is not linear; that is why the callback receives and returns a whole packages array: packages = configuration.filterPackages(packages).)

brettz9 commented 4 years ago

I haven't added a test yet as I thought I'd make sure the API approach is ok. For those wishing to see an example, this is how I'm using it (bundledRootPackages is the whitelist of devDeps to check):

filterPackages (unflattenedPackages) {
    const packages = [];
    const flatten = (pkg) => {
      packages.push(pkg);
      if (pkg.children) {
        pkg.children.forEach((p) => flatten(p));
      }
    };
    unflattenedPackages.forEach((p) => flatten(p));

    // console.log('packages', packages);
    const filteredPackages = packages.filter((pkg) => {
      // Ensure we are getting a package with the version set in the
      //  user's package.json
      // Could also be a normal dep. if, e.g., copying for browser;
      //   but normally will be whitelisting devDep. that we are copying
      //   over
      // const isRootDep = pkg.package._requiredBy.includes('#USER');
      const isRootDevDep = pkg.package._requiredBy.includes('#DEV:/');
      return isRootDevDep && bundledRootPackages.includes(pkg.name);
    });

    // eslint-disable-next-line jsdoc/require-jsdoc
    function getDeps (pkgs) {
      pkgs.forEach((pkg) => {
        if (!pkg) {
          return;
        }
        const {package: {dependencies}} = pkg;
        if (dependencies) {
          const pkgsToCheck = [];
          Object.keys(dependencies).forEach((dep) => {
            const findPkg = (pk) => {
              if (!pk) {
                return false;
              }
              const {name} = pk;
              return dep === name;
            };
            /* eslint-disable unicorn/no-fn-reference-in-iterator */
            if (filteredPackages.find(findPkg) !== undefined) {
              return;
            }
            const pk = packages.find(findPkg);
            /* eslint-enable unicorn/no-fn-reference-in-iterator */
            pkgsToCheck.push(pk);
            filteredPackages.push(pk);
          });
          getDeps(pkgsToCheck);
        }
      });
    }

    getDeps(filteredPackages);

    return filteredPackages;
  }
brettz9 commented 4 years ago

@kemitchell : Hi--was just wondering if I could ask--did you have reservations about adding this, or do you just need some time to review?

kemitchell commented 4 years ago

There was discussion in #53, but I thought I remembered comments from others, too.

If you need this functionality now, by all means run a fork. @brettz/licensee on the public registry isn't going to irk me or anyone else.

I'm very grateful for this PR, especially seeing how politely and respectfully you sent it. But my personal priority for this package, to the extent I have time, is all the change dependencies are going to force on us pretty soon. See #64.

kemitchell commented 4 years ago

See Also: #54. A few of @ljharb's comments there apply to #53, methinks. But that's not to say this isn't welcome. But I am thinking this gets superseded by rewriting we're going to have to do for Arborist.

brettz9 commented 4 years ago

Thank you for the status update, @kemitchell . As far as @ljharb 's comments on legal liability, that doesn't apply in my case. I am not seeking to ignore transitive dependencies (on the contrary, for the packages being bundled, I want to include all transitive dependencies), but instead am checking a subset of devDependencies that are being bundled.

In a number of repos, I use npm for versioning, but have a copy script to copy devDependencies code into the project proper. (The reason for this is to allow Github-based hosting services like Github Pages to include npm files by making them a part of the repo.)

In such repos, if I were to only look at dependencies, using the library now (at least if checking dependencies only) would ignore some code that actually makes it into a distribution. But checking all devDependencies is overly aggressive in our case, as that includes many packages not being bundled and therefore not applicable to us. So my concern is actually to be more careful in checking liability.

kemitchell commented 4 years ago

@brettz9 the license terms of some devDependencies may affect what you want to do, even if you don't bundle or redistribute their code. For example, projects under RPL, Parity, or nonstandard, custom terms.

brettz9 commented 4 years ago

Sure, and that is good to know, but one can have a separate process for checking that. I want to indicate what is being bundled. One can already opt to only check dependencies. My change offers an additive option.

kemitchell commented 4 years ago

It sounds like you don't want a filter so much as to control all the input to a "do these things meet the license rule" algorithm.

brettz9 commented 4 years ago

Yes, you could put it that way. I think I am technically filtering to only those devDeps of interest, but yes, that is what I'm seeking to do (and why the rest of your code is still helpful to me as it does that).

brettz9 commented 4 years ago

The filter in this PR is filtering those at root which end up getting checked, not filtering out transitive deps along the way.

brettz9 commented 4 years ago

As this just adds a single boolean check for existing users, I'm wondering whether you would be ok to merge this? It'd be great not to have to publish a separate fork just for this if this use case could be accommodated. As mentioned, I've been using https://github.com/brettz9/license-badger in various projects based on this, and has been working rather well.

I'm happy to rename the config to filterRootPackages if that helps.

kemitchell commented 4 years ago

v8.1.0