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

filterPackages callback option #63

Merged
merged 1 commit into from May 17, 2020

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Nov 27, 2019

  • In programmatic licensee, allow filterPackages callback to filter which packages have their children processed

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
Copy link
Contributor Author

brettz9 commented Nov 27, 2019

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
Copy link
Contributor Author

brettz9 commented Dec 12, 2019

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

brettz9 commented Dec 14, 2019

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
Copy link
Member

@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
Copy link
Contributor Author

brettz9 commented Dec 14, 2019

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
Copy link
Member

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
Copy link
Contributor Author

brettz9 commented Dec 14, 2019

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
Copy link
Contributor Author

brettz9 commented Dec 14, 2019

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

… which packages have their children processed
@brettz9
Copy link
Contributor Author

brettz9 commented May 17, 2020

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 kemitchell merged commit 4d763f4 into jslicense:master May 17, 2020
@kemitchell
Copy link
Member

v8.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whitelist of packages to include
2 participants