Add support for cyclical JS dependencies

README.md

@@ -19,7 +19,6 @@ This plugin changes quite a few things in the Browserify pipeline to make CSS wo
 In particular, the following things should be kept in mind:
 
 - This plugin changes Browserify's sorting algorithm, so that files are always processed in 'dependency order'. While this *shouldn't* be an issue because the new algorithm is deterministic just like the old one, it's possible for a different plugin to break this one, if it changes the sorting algorithm. To prevent this, always load `icssify` __last__ (but still before `watchify` and `css-extract`, if you're using those).
-- The sorting algorithm cannot currently deal with circular dependencies. __Trying to use this plugin will break, when you have circular dependencies.__ You really shouldn't have those, but this is still worth keeping in mind. PRs to fix this are welcome.
 - CSS files are not JS files. This plugin sneaks the CSS past Browserify's syntax checker, but it will still send CSS files through the pipeline. Plugins that operate on JS *must* ignore non-JS files, otherwise they will break.
 
 This plugin will always bundle all CSS __into a single file__. For complexity reasons, there is currently no support for splitting up CSS into multiple bundles. PRs that add support for this (without breaking ICSS or `css-extract` support!) are welcome.
@@ -74,10 +73,16 @@ Plugin options (all optional):
 * __mode:__ Whether to assume that untagged class names in your CSS (ie. those without a `:local` or `:global` tag) are local or global. Defaults to `"local"`, but you can set this to `"global"` if you want to make the class name mangling *opt-in*. You'll generally want to leave this at the default setting.
 * __before:__ PostCSS transforms to run *before* the ICSS transforms, ie. before imports/exports are analyzed. This is usually where you want custom PostCSS plugins to go.
 * __after:__ PostCSS transforms to run *after* the ICSS transforms, ie. after mangling the class names, but before bundling it all together into a single file. You'll rarely need to use this.
+* __ignoreCycles:__ When enabling this, `icssify` will handle cyclical dependencies by randomly (but deterministically) ignoring a dependency relation during sorting. This *should* be safe to do for JS files, but there's no guarantee - and if things break in strange ways when you enable this, that is probably why.
+  __Cyclical dependencies in CSS files are *still* not allowed__, even when enabling this setting! It's just intended to deal with cyclical JS dependencies during sorting, which are a bad practice but technically valid to have. It's opt-in to ensure that you understand the risks of enabling it.
 
 
 ## Changelog
 
+### v1.3.0 (June XX, 2020)
+
+- __Feature:__ Added more sensible handling of cyclical dependencies. It will now throw a clear error instead of silently dropping modules on the floor, and gives you the `ignoreCycles` option to continue bundling anyway.
+
 ### v1.2.1 (March 6, 2020)
 
 - __Documentation:__ Fixed missing changelog item for v1.2.0.

index.js

@@ -1,8 +1,6 @@
 "use strict";
 
-const { validateArguments, required, oneOf, arrayOf, isString, allowExtraProperties, ValidationError } = require("validatem");
-
-const createKahnSortingStream = require("./src/phase-streams/sort-kahn");
+const { validateArguments, required, oneOf, arrayOf, isString, isBoolean, allowExtraProperties, ValidationError } = require("validatem");
 
 function isPostcssPlugin(value) {
 	if (value.postcssPlugin == null || value.postcssVersion == null || typeof value !== "function") {
@@ -14,21 +12,26 @@ function isPostcssPlugin(value) {
 
 module.exports = function (browserify, options) {
 	validateArguments(arguments, [
-		[ "browserify" ],
+		[ "browserify", required ],
 		[ "options", allowExtraProperties({
 			mode: oneOf([ "local", "global" ]),
 			before: arrayOf([ required, isPostcssPlugin ]),
 			after: arrayOf([ required, isPostcssPlugin ]),
-			extensions: arrayOf([ required, isString ])
+			extensions: arrayOf([ required, isString ]),
+			ignoreCycles: isBoolean
 		})]
 	]);
-
-	let state = { extensions: options.extensions };
+	
+	let state = {
+		extensions: options.extensions,
+		ignoreCycles: options.ignoreCycles
+	};
 
 	const createTransform = require("./src/transform")(state);
 	const createCssDepsStream = require("./src/phase-streams/deps-css")(state);
 	const createSyntaxHideStream = require("./src/phase-streams/syntax-hide")(state);
 	const createSyntaxUnhideStream = require("./src/phase-streams/syntax-unhide")(state);
+	const createKahnSortingStream = require("./src/phase-streams/sort-kahn")(state);
 	const createDedupeBundleCssStream = require("./src/phase-streams/dedupe-bundle-css")(state);
 
 	function setupPipeline() {

package.json

@@ -26,6 +26,7 @@
     "bl": "^4.0.0",
     "bluebird": "^3.7.1",
     "default-value": "^1.0.0",
+    "find-cycle": "^1.0.0",
     "generic-names": "^2.0.1",
     "insert-css": "^2.0.0",
     "object.fromentries": "^2.0.1",

src/cycle-error.js

@@ -0,0 +1,5 @@
+"use strict";
+
+const createError = require("create-error");
+
+module.exports = createError("CycleError");

src/find-cycle-sets.js

@@ -0,0 +1,40 @@
+"use strict";
+
+const findCycle = require("find-cycle/directed");
+
+module.exports = function findCycleSets(nodes) {
+	let queue = nodes.slice();
+	let cycleSets = [];
+	let innocentNodes = [];
+	let lastQueueLength = queue.length; 
+
+	while (queue.length > 0) {
+		let queueSet = new Set(queue);
+
+		let cycle = findCycle(queue, (node) => {
+			return node.children.filter((child) => {
+				// We want to prevent findCycle from discovering and traversing any nodes that are not in our "conflict set"; otherwise a second pass will fail, because it will rediscover already-handled nodes (that have been removed from the queue) indirectly, and endlessly loop on discovering that cycle.
+				return queueSet.has(child);
+			});
+		});
+		
+		if (cycle != null) {
+			cycleSets.push(cycle);
+			queue = queue.filter((node) => !cycle.includes(node));
+
+			if (queue.length === lastQueueLength) {
+				throw new Error(`Failed to reduce queue length; this is a bug, please report it!`);
+			} else {
+				lastQueueLength = queue.length;
+			}
+		} else {
+			innocentNodes = queue;
+			break;
+		}
+	}
+
+	return {
+		cycleSets: cycleSets,
+		innocentNodes: innocentNodes
+	};
+};

src/kahn.js

@@ -2,6 +2,8 @@
 
 const util = require("util");
 
+const CycleError = require("./cycle-error");
+
 // TODO: Publish this as a stand-alone module
 
 module.exports = function kahn(nodes) {
@@ -36,5 +38,14 @@ module.exports = function kahn(nodes) {
 		}
 	}
 
+	if (list.length !== nodes.length) {
+		let processedNodes = new Set(list);
+		let affectedNodes = nodes.filter((node) => !processedNodes.has(node));
+
+		throw new CycleError(`One or more cycles were detected, involving ${affectedNodes.length} nodes`, {
+			affectedNodes: affectedNodes
+		});
+	}
+
 	return list;
 };

src/phase-streams/sort-kahn.js

@@ -3,19 +3,21 @@
 const sortDependencies = require("../sort-dependencies");
 const stream = require("../stream");
 
-module.exports = function createKahnSortingStream() {
-	let sortables = [];
+module.exports = function({ ignoreCycles }) {
+	return function createKahnSortingStream() {
+		let sortables = [];
 
-	function handleItem(item) {
-		sortables.push(item);
-	}
+		function handleItem(item) {
+			sortables.push(item);
+		}
 
-	function flush() {
-		let sorted = sortDependencies(sortables);
+		function flush() {
+			let sorted = sortDependencies(sortables, {}, { ignoreCycles });
 
-		// TODO: Verify that the 'null' push here is necessary
-		return sorted.concat([ null ]);
-	}
+			// TODO: Verify that the 'null' push here is necessary
+			return sorted.concat([ null ]);
+		}
 
-	return stream(handleItem, flush);
+		return stream(handleItem, flush);
+	};
 };

src/sort-dependencies.js

@@ -1,8 +1,87 @@
 "use strict";
 
 const kahn = require("./kahn");
+const CycleError = require("./cycle-error");
+const findCycleSets = require("./find-cycle-sets");
+
+function printNodes(nodes) {
+	return nodes
+		.map((node) => node.id)
+		.map((id) => ` - ${id}`)
+		.join("\n");
+}
+
+function mergeRemovedEdges(a, b) {
+	let newObject = {};
+
+	function addEdges(key, values) {
+		if (newObject[key] == null) {
+			newObject[key] = values;
+		} else {
+			newObject[key] = newObject[key].concat(values);
+		}
+	}
+
+	for (let [ key, value ] of Object.entries(a)) {
+		addEdges(key, value);
+	}
+
+	for (let [ key, value ] of Object.entries(b)) {
+		addEdges(key, value);
+	}
+
+	return newObject;
+}
+
+function findEdgesToRemove(affectedNodes) {
+	let { cycleSets } = findCycleSets(affectedNodes);
+	let edgesToRemove = {};
+
+	function addEdge(key, value) {
+		if (edgesToRemove[key] == null) {
+			edgesToRemove[key] = [];
+		}
+
+		edgesToRemove[key].push(value);
+	}
+
+	for (let cycle of cycleSets) {
+		let firstModule = cycle[0];
+		let firstConflictingModule = cycle.find((module_) => firstModule.parents.includes(module_));
+
+		addEdge(firstModule.id, firstConflictingModule.id);
+	}
+
+	return edgesToRemove;
+}
+
+function createDependencyCycleError(affectedNodes) {
+	let { cycleSets, innocentNodes } = findCycleSets(affectedNodes);
+			
+	let cycleSetStrings = cycleSets.map((set) => printNodes(set));
+
+	let innocentNodeString = (innocentNodes.length === 0)
+		? " (none)"
+		: printNodes(innocentNodes);
+
+	let message = [
+		`At least ${cycleSets.length} circular dependency chain(s) were detected, involving the following modules:`,
+		"",
+		cycleSetStrings.join("\n\n"),
+		"",
+		"Additionally, the following modules could not be processed as a result:",
+		"",
+		innocentNodeString,
+		"",
+		"Please read the icssify documentation for the 'ignoreCycles' option to understand how to deal with this."
+	].join("\n");
+
+	return new CycleError(message);
+}
+
+module.exports = function sortDependencies(items, removedEdges, options = {}) {
+	let { ignoreCycles } = options;
 
-module.exports = function sortDependencies(items) {
 	let dependencyMap = new Map();
 	let nodeMap = new Map();
 	
@@ -14,20 +93,45 @@ module.exports = function sortDependencies(items) {
 	});
 
 	items.forEach((item) => {
+		let removedEdgesForItem = new Set(removedEdges[item.id]);
+
 		Object.values(item.deps)
 			.filter((dep => dep !== item.id))
 			.forEach((dep) => {
-				nodeMap.get(dep).children.push(nodeMap.get(item.id));
+				let parent = nodeMap.get(dep);
+
+				if (removedEdgesForItem == null || !removedEdgesForItem.has(parent.id)) {
+					parent.children.push(nodeMap.get(item.id));
+				}
 			});
 
 		nodeMap.get(item.id).parents = Object.values(item.deps)
 			.filter((id => id !== item.id))
-			.map((id) => nodeMap.get(id));
+			.map((id) => nodeMap.get(id))
+			.filter((node) => !removedEdgesForItem.has(node.id));
 	});
 	
-	let sortedNodes = kahn(Array.from(nodeMap.values()));
-	
-	return sortedNodes.map((node) => {
-		return dependencyMap.get(node.id);
-	});
+	try {
+		let sortedNodes = kahn(Array.from(nodeMap.values()));
+
+		return sortedNodes.map((node) => {
+			return dependencyMap.get(node.id);
+		});
+	} catch (error) {
+		if (error instanceof CycleError) {
+			if (ignoreCycles) {
+				// TODO: Disallow cycles for extensions processed by icssify, *even* when this option is active, because cycles just aren't (sensibly) possible in ICSS
+				let newRemovedEdges = findEdgesToRemove(error.affectedNodes);
+
+				// TODO: Debug-log removed edges here, and the full cycle, to debug potential future infinite recursion bugs
+
+				// We will keep throwing out more (deterministically) random edges until we're left with no cycles. If there are enough cycles to cause an exceeded stack size here, you probably have bigger issues to worry about...
+				return sortDependencies(items, mergeRemovedEdges(removedEdges, newRemovedEdges), options);
+			} else {
+				throw createDependencyCycleError(error.affectedNodes);
+			}
+		} else {
+			throw error;
+		}
+	}
 };