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

WIP: Refactor loader dependencies #119

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .babelrc
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"presets": [
["env", {"targets": {"node": "6"}}]
["@babel/preset-env", {"targets": {"node": "6"}}]
]
}
9 changes: 5 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"url": "https://github.com/css-modules/postcss-modules.git"
},
"dependencies": {
"@babel/register": "^7.12.10",
"generic-names": "^2.0.1",
"icss-replace-symbols": "^1.1.0",
"lodash.camelcase": "^4.3.0",
Expand All @@ -31,11 +32,11 @@
"postcss": "^8.0.0"
},
"devDependencies": {
"@babel/cli": "7.12.10",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

"@babel/core": "7.12.10",
"@babel/preset-env": "7.12.11",
"autoprefixer": "^10.0.2",
"babel-cli": "^6.26.0",
"babel-core": "^6.26.3",
"babel-eslint": "^10.1.0",
"babel-preset-env": "^1.7.0",
"eslint": "^7.3.1",
"eslint-plugin-import": "^2.21.2",
"eslint-plugin-jest": "^23.17.0",
Expand Down Expand Up @@ -64,5 +65,5 @@
"git add"
]
},
"require": "babel-core/register"
"require": "@babel/register"
}
118 changes: 19 additions & 99 deletions src/css-loader-core/loader.js
Original file line number Diff line number Diff line change
@@ -1,112 +1,32 @@
// Copied from https://github.com/css-modules/css-modules-loader-core
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can move the loader and parser files to the src directory directly. The directory was just a copy-and-paste from the library as a hot fix and unnecessary now.


import postcss from "postcss";
import fs from "fs";
import path from "path";

import Parser from "./parser";

class Core {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love to see the Core is gone

constructor(plugins) {
this.plugins = plugins || Core.defaultPlugins;
export function resolveRelativeImport(importee, importer, projectRoot) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be an internal function. Let's not export it then

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, of course. good catch :)

const importerDir = path.dirname(importer);
let pathFromProjectRoot = path.resolve(projectRoot, importerDir, importee);

// if the path is not relative or absolute, try to resolve it in node_modules
if (importee[0] !== "." && importee[0] !== "/") {
try {
pathFromProjectRoot = require.resolve(importee);
} catch (e) {
// noop
}
}

load(sourceString, sourcePath, trace, pathFetcher) {
let parser = new Parser(pathFetcher, trace);

return postcss(this.plugins.concat([parser.plugin()]))
.process(sourceString, { from: "/" + sourcePath })
.then((result) => {
return {
injectableSource: result.css,
exportTokens: parser.exportTokens,
};
});
}
return pathFromProjectRoot;
}

// Sorts dependencies in the following way:
// AAA comes before AA and A
// AB comes after AA and before A
// All Bs come after all As
// This ensures that the files are always returned in the following order:
// - In the order they were required, except
// - After all their dependencies
const traceKeySorter = (a, b) => {
if (a.length < b.length) {
return a < b.substring(0, a.length) ? -1 : 1;
} else if (a.length > b.length) {
return a.substring(0, b.length) <= b ? -1 : 1;
} else {
return a < b ? -1 : 1;
}
};

export default class FileSystemLoader {
constructor(root, plugins) {
this.root = root;
this.sources = {};
this.traces = {};
this.importNr = 0;
this.core = new Core(plugins);
this.tokensByFile = {};
}

fetch(_newPath, relativeTo, _trace) {
let newPath = _newPath.replace(/^["']|["']$/g, ""),
trace = _trace || String.fromCharCode(this.importNr++);
export default {
resolveId: (importee, importer, projectRoot) => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea to use resolveId, it may be really useful in the future

return resolveRelativeImport(importee, importer, projectRoot)
},
resolve: (importee) => {
return new Promise((resolve, reject) => {
let relativeDir = path.dirname(relativeTo),
rootRelativePath = path.resolve(relativeDir, newPath),
fileRelativePath = path.resolve(
path.join(this.root, relativeDir),
newPath
);
Comment on lines -59 to -64
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madyankin
Btw, do you have a bit of insight on the old code? I'm not quite sure about the distinction between rootRelativePath and fileRelativePath here. They are equal as long as this.root is unset and I don't see why they need to be be different in the alternative case... so while refactoring I got rid of the distinction.
But since I don't completely trust the test suite anymore, I'm not sure if that's a correct step to take. Any idea?

Copy link
Owner

@madyankin madyankin Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I had the insight a couple of years ago or so, but I'm not quite sure now.

AFAIR, this is useful when you have CSS modules placed in some other location and want to resolve them properly, e.g., in Rails or Django frameworks.

These files were in the css-modules-loader-core package without tests. Given I copied them here as a hotfix, I hadn't enough time to cover them with.


// if the path is not relative or absolute, try to resolve it in node_modules
if (newPath[0] !== "." && newPath[0] !== "/") {
try {
fileRelativePath = require.resolve(newPath);
} catch (e) {
// noop
}
}

const tokens = this.tokensByFile[fileRelativePath];
if (tokens) {
return resolve(tokens);
}

fs.readFile(fileRelativePath, "utf-8", (err, source) => {
fs.readFile(importee, "utf-8", (err, source) => {
if (err) reject(err);
this.core
.load(source, rootRelativePath, trace, this.fetch.bind(this))
.then(({ injectableSource, exportTokens }) => {
this.sources[fileRelativePath] = injectableSource;
this.traces[trace] = fileRelativePath;
this.tokensByFile[fileRelativePath] = exportTokens;
resolve(exportTokens);
}, reject);
else resolve(source);
});
});
}

get finalSource() {
const traces = this.traces;
const sources = this.sources;
let written = new Set();

return Object.keys(traces)
.sort(traceKeySorter)
.map((key) => {
const filename = traces[key];
if (written.has(filename)) {
return null;
}
written.add(filename);

return sources[filename];
})
.join("");
}
}
101 changes: 82 additions & 19 deletions src/css-loader-core/parser.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
// Copied from https://github.com/css-modules/css-modules-loader-core
import replaceSymbols from "icss-replace-symbols";
import postcss from 'postcss';

const importRegexp = /^:import\((.+)\)$/;
import replaceSymbols from "icss-replace-symbols";

export default class Parser {
constructor(pathFetcher, trace) {
this.pathFetcher = pathFetcher;
constructor(root, plugins, loader, trace, ctx = {}) {
this.root = root;
this.plugins = plugins;
this.loader = loader;
this.plugin = this.plugin.bind(this);
this.exportTokens = {};
this.translations = {};
this.trace = trace;
this.trace = trace || '';

this.sources = ctx.sources || {};
this.traces = ctx.traces || {};
this.tokensByFile = ctx.tokensByFile || {};
}

plugin() {
Expand All @@ -26,10 +33,11 @@ export default class Parser {

fetchAllImports(css) {
let imports = [];
let trace = 0;
css.each((node) => {
if (node.type == "rule" && node.selector.match(importRegexp)) {
imports.push(
this.fetchImport(node, css.source.input.from, imports.length)
this.fetchImport(node, css.source.input.from, trace++)
);
}
});
Expand Down Expand Up @@ -62,19 +70,74 @@ export default class Parser {
exportNode.remove();
}

fetchImport(importNode, relativeTo, depNr) {
let file = importNode.selector.match(importRegexp)[1],
depTrace = this.trace + String.fromCharCode(depNr);
return this.pathFetcher(file, relativeTo, depTrace).then(
(exports) => {
importNode.each((decl) => {
if (decl.type == "decl") {
this.translations[decl.prop] = exports[decl.value];
}
});
importNode.remove();
},
(err) => console.log(err)
);
async fetchImport(importNode, relativeTo, depNr) {
const file = importNode.selector.match(importRegexp)[1].replace(/^["']|["']$/g, ""),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's split the declarations into separate statements everywhere for the readability sake

depTrace = this.trace + String.fromCharCode(65 + depNr), // 65 = A, making the trace more readable
id = this.loader.resolveId(file, relativeTo, this.root),
subParser = new Parser(this.root, this.plugins, this.loader, depTrace, {
sources: this.sources,
traces: this.traces,
tokensByFile: this.tokensByFile
});
let tokens = this.tokensByFile[id];

try {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be cleaner now 👍

if (!tokens) {
const content = await this.loader.resolve(id);
const { css } = await postcss(this.plugins.concat([subParser.plugin()]))
.process(content, { from: id });
tokens = subParser.exportTokens;

this.sources[id] = css;
this.traces[depTrace] = id;
this.tokensByFile[id] = tokens;
}

importNode.each((decl) => {
if (decl.type == "decl") {
this.translations[decl.prop] = tokens[decl.value];
}
});
importNode.remove();
} catch (e) {
console.error(e);
}
}


get finalSource() {
const traces = this.traces;
const sources = this.sources;
let written = new Set();

return Object.keys(traces)
.sort(traceKeySorter)
.map((key) => {
const filename = traces[key];
if (written.has(filename)) {
return null;
}
written.add(filename);

return sources[filename];
})
.join("");
}
}

// Sorts dependencies in the following way:
// AAA comes before AA and A
// AB comes after AA and before A
// All Bs come after all As
// This ensures that the files are always returned in the following order:
// - In the order they were required, except
// - After all their dependencies
const traceKeySorter = (a, b) => {
if (a.length < b.length) {
return a < b.substring(0, a.length) ? -1 : 1;
} else if (a.length > b.length) {
return a.substring(0, b.length) <= b ? -1 : 1;
} else {
return a < b ? -1 : 1;
}
};
16 changes: 7 additions & 9 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import camelCase from "lodash.camelcase";
import genericNames from "generic-names";

import Parser from "./css-loader-core/parser";
import FileSystemLoader from "./css-loader-core/loader";
import defaultLoader from "./css-loader-core/loader";

import generateScopedName from "./generateScopedName";
import saveJSON from "./saveJSON";
Expand All @@ -29,11 +29,8 @@ function getScopedNameGenerator(opts) {
});
}

function getLoader(opts, plugins) {
const root = typeof opts.root === "undefined" ? "/" : opts.root;
return typeof opts.Loader === "function"
? new opts.Loader(root, plugins)
: new FileSystemLoader(root, plugins);
function getLoader(opts) {
return opts.loader || defaultLoader;
}

function isGlobalModule(globalModules, inputFile) {
Expand Down Expand Up @@ -84,14 +81,15 @@ module.exports = (opts = {}) => {
}
const earlierPlugins = result.processor.plugins.slice(0, resultPluginIndex);
const loaderPlugins = [...earlierPlugins, ...pluginList];
const loader = getLoader(opts, loaderPlugins);
const parser = new Parser(loader.fetch.bind(loader));
const loader = getLoader(opts);
const root = typeof opts.root === "undefined" ? "/" : opts.root;
const parser = new Parser(root, loaderPlugins, loader);

await postcss([...pluginList, parser.plugin()]).process(css, {
from: inputFile,
});

const out = loader.finalSource;
const out = parser.finalSource;
if (out) css.prepend(out);

if (opts.localsConvention) {
Expand Down