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

Convert npm-discards.js to TypeScript #10663

Open
wants to merge 9 commits into
base: release-1.8.2
from
@@ -90,7 +90,7 @@ tools/isobuild/js-analyze.js
tools/isobuild/linker.js
tools/isobuild/linter-plugin.js
tools/isobuild/meteor-npm.js
tools/isobuild/npm-discards.js
tools/isobuild/npm-discards.ts
tools/isobuild/package-api.js
tools/isobuild/package-source.js
tools/isobuild/source-arch.js
@@ -17,4 +17,10 @@ declare global {
// func-utils.ts makes usage of this feature
displayName?: string;
}

type StringOrRegExp = string | RegExp;

interface Discards {
[packageName: string]: StringOrRegExp[];
}
}
@@ -3,7 +3,7 @@ import {WatchSet, readAndWatchFile, sha1} from '../fs/watch';
import files, {
symlinkWithOverwrite,
} from '../fs/files';
import NpmDiscards from './npm-discards.js';
import NpmDiscards from './npm-discards';
import {Profile} from '../tool-env/profile';
import {
optimisticReadFile,

This file was deleted.

@@ -0,0 +1,93 @@
import * as files from "../fs/files";

const hasOwn = Object.prototype.hasOwnProperty;

// This class encapsulates a structured specification of files and
// directories that should be stripped from the node_modules directories
// of Meteor packages during `meteor build`, as requested by calling
// `Npm.discard` in package.js files.
class NpmDiscards {
private discards: Discards;

constructor() {
this.discards = {};
}

// Update the current specification of discarded files with additional
// patterns that should be discarded. See the comment in package-source.js
// about `Npm.strip` for an explanation of what should be passed for the
// `discards` parameter.
merge(discards: Discards) {
merge(this.discards, discards);
}

shouldDiscard(candidatePath: string, isDirectory?: boolean) {
if (isDirectory === void 0) {
isDirectory = files.lstat(candidatePath).isDirectory();
}

for (let currentPath = candidatePath, parentPath;
(parentPath = files.pathDirname(currentPath)) !== currentPath;
currentPath = parentPath) {
if (files.pathBasename(parentPath) === "node_modules") {
const packageName = files.pathBasename(currentPath);

if (hasOwn.call(this.discards, packageName)) {
let relPath = files.pathRelative(currentPath, candidatePath);

if (isDirectory) {
relPath = files.pathJoin(relPath, files.pathSep);
}

return this.discards[packageName].some(pattern =>
matches(pattern, relPath)
);
}

// Stop at the first ancestor node_modules directory we find.
break;
}
}

return false;
}
}

function merge(into: Discards, from: Discards) {
Object.keys(from).forEach((packageName: string) => {
const fromValue = from[packageName];
const intoValue = hasOwn.call(into, packageName) && into[packageName];

if (typeof fromValue === "string" || fromValue instanceof RegExp) {
This conversation was marked as resolved by jamesmillerburgess

This comment has been minimized.

Copy link
@jamesmillerburgess

jamesmillerburgess Aug 4, 2019

Author Contributor

I'm not sure the code in this if block is reachable, since the values are always supposed to be arrays, according to this comment in package-npm.js:

  // The `discards` parameter should be an object whose keys are
  // top-level package names and whose values are arrays of strings
  // (or regular expressions) that match paths in that package's
  // directory that should be stripped before installation. For
  // example:
  //
  //   Npm.strip({
  //     connect: [/*\.wmv$/],
  //     useragent: ["tests/"]
  //   });

But I wanted to check before deleting it.

This comment has been minimized.

Copy link
@jamesmillerburgess

jamesmillerburgess Aug 4, 2019

Author Contributor

For what it's worth, I logged the results in the if and else blocks while building our production application and got 8 arrays and 0 string/RegExps:

console.log("I'm an array! ", fromValue, from);
I'm an array!  [ 'test/' ] { 'es5-ext': [ 'test/' ] }
I'm an array!  [ 'test/' ] { 'es5-ext': [ 'test/' ] }
I'm an array!  [ 'test/' ] { mongodb: [ 'test/' ] }
I'm an array!  [ 'test/' ] { mongodb: [ 'test/' ] }
I'm an array!  [ 'test/' ] { multiparty: [ 'test/' ], useragent: [ 'test/' ] }
I'm an array!  [ 'test/' ] { multiparty: [ 'test/' ], useragent: [ 'test/' ] }
I'm an array!  [ 'test/' ] { multiparty: [ 'test/' ], useragent: [ 'test/' ] }
I'm an array!  [ 'test/' ] { multiparty: [ 'test/' ], useragent: [ 'test/' ] }

And I got the same result with ./meteor test-packages.

This comment has been minimized.

Copy link
@jamesmillerburgess

jamesmillerburgess Aug 4, 2019

Author Contributor

I looked through all 19 pages of GitHub results for npm.strips in package.js files and all of them use the array syntax:

https://github.com/search?l=&p=19&q=%22npm.strip%22+filename%3Apackage.js&ref=advsearch&type=Code&utf8=✓

This comment has been minimized.

Copy link
@jamesmillerburgess

jamesmillerburgess Aug 6, 2019

Author Contributor

After thinking about this some more, maybe it would be better to support both formats, but then I should update the misleading comment about NPM.strip and also update the type for Discards to accommodate both.

if (intoValue) {
intoValue.push(fromValue);
} else {
into[packageName] = [fromValue];
}

} else if (Array.isArray(fromValue)) {
if (intoValue) {
intoValue.push.apply(intoValue, fromValue);
} else {
// Make a defensive copy of any arrays passed to `Npm.strip`.
into[packageName] = fromValue.slice(0);
}
}
});
}

// TODO Improve this. For example we don't currently support wildcard
// string patterns (just use a RegExp if you need that flexibility).
function matches(pattern: StringOrRegExp, relPath: string) {
if (pattern instanceof RegExp) {
return relPath.match(pattern);
}

if (pattern.charAt(0) === files.pathSep) {
return relPath.indexOf(pattern.slice(1)) === 0;
}

return relPath.includes(pattern);
}

export default NpmDiscards;
@@ -1,6 +1,6 @@
import { ensureOnlyValidVersions } from "../utils/utils.js";
import buildmessage from "../utils/buildmessage.js";
import NpmDiscards from "./npm-discards.js";
import NpmDiscards from "./npm-discards";

const nodeRequire = require;

@@ -0,0 +1,135 @@
import { ensureOnlyValidVersions } from "../utils/utils.js";
import buildmessage from "../utils/buildmessage.js";
import NpmDiscards from "./npm-discards";

const nodeRequire = require;

interface Dependencies {
[packageName: string]: string;
}

interface Discards {
[packageName: string]: (string | RegExp)[];
}

export class PackageNpm {
private _dependencies: Dependencies | null;

/**
* @summary Class of the 'Npm' object visible in package.js
* @locus package.js
* @instanceName Npm
* @showInstanceName true
*/
constructor() {
// Files to be stripped from the installed NPM dependency tree. See
// the Npm.strip comment below for further usage information.
this._discards = new NpmDiscards;
this._dependencies = null;
}

/**
* @summary Specify which [NPM](https://www.npmjs.org/) packages
* your Meteor package depends on.
* @param {Object} dependencies An object where the keys are package
* names and the values are one of:
* 1. Version numbers in string form
* 2. http(s) URLs of npm packages
* 3. Git URLs in the format described [here](https://docs.npmjs.com/files/package.json#git-urls-as-dependencies)
*
* Https URL example:
*
* ```js
* Npm.depends({
* moment: "2.8.3",
* async: "https://github.com/caolan/async/archive/71fa2638973dafd8761fa5457c472a312cc820fe.tar.gz"
* });
* ```
*
* Git URL example:
*
* ```js
* Npm.depends({
* moment: "2.8.3",
* async: "git+https://github.com/caolan/async#master"
* });
* ```
* @locus package.js
*/
depends(dependencies: Dependencies) {
// XXX make dependencies be separate between use and test, so that
// production doesn't have to ship all of the npm modules used by test
// code
if (this._dependencies) {
buildmessage.error("Npm.depends may only be called once per package",
{ useMyCaller: true });
// recover by ignoring the Npm.depends line
return;
}

if (typeof dependencies !== 'object') {
buildmessage.error("the argument to Npm.depends should be an " +
"object, like this: {gcd: '0.0.0'}",
{ useMyCaller: true });
// recover by ignoring the Npm.depends line
return;
}

// don't allow npm fuzzy versions so that there is complete
// consistency when deploying a meteor app
//
// XXX use something like seal or lockdown to have *complete*
// confidence we're running the same code?
try {
ensureOnlyValidVersions(dependencies, {
forCordova: false
});

} catch (e) {
buildmessage.error(e.message, {
useMyCaller: true,
downcase: true
});

// recover by ignoring the Npm.depends line
return;
}

this._dependencies = dependencies;
}

// The `Npm.strip` method makes up for packages that have missing
// or incomplete .npmignore files by telling the bundler to strip out
// certain unnecessary files and/or directories during `meteor build`.
//
// The `discards` parameter should be an object whose keys are
// top-level package names and whose values are arrays of strings
// (or regular expressions) that match paths in that package's
// directory that should be stripped before installation. For
// example:
//
// Npm.strip({
// connect: [/*\.wmv$/],
// useragent: ["tests/"]
// });
//
// This means (1) "remove any files with the `.wmv` extension from
// the 'connect' package directory" and (2) "remove the 'tests'
// directory from the 'useragent' package directory."
strip(discards: Discards) {
this._discards.merge(discards);
}

require(name: string) {
try {
return nodeRequire(name); // from the dev bundle
} catch (e) {
buildmessage.error(
"can't find npm module '" + name +
"'. In package.js, Npm.require can only find built-in modules.",
{ useMyCaller: true });
// recover by, uh, returning undefined, which is likely to
// have some knock-on effects
}
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.