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

feat: added css to packem add command #53

Merged
merged 4 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions examples/styles/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
},
"devDependencies": {
"@visulima/packem": "workspace:*",
"cssnano": "7.0.6",
"esbuild": "^0.24.0",
"icss-utils": "^5.1.0",
"less": "^4.2.0",
"postcss": "^8.4.47",
"postcss-load-config": "^6.0.1",
Expand All @@ -30,6 +32,7 @@
"postcss-value-parser": "^4.2.0",
"rimraf": "^6.0.1",
"sass-embedded": "^1.80.6",
"stylus": "^0.64.0",
"typescript": "^5.6.3"
}
}
12 changes: 7 additions & 5 deletions examples/styles/packem.config.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
import { defineConfig } from "@visulima/packem/config";
import transformer from "@visulima/packem/transformer/esbuild";
import lessLoader from "@visulima/packem/css/loader/less";
import postcssLoader from "@visulima/packem/css/loader/postcss";
import sassLoader from "@visulima/packem/css/loader/sass";
import stylusLoader from "@visulima/packem/css/loader/stylus";
import lessLoader from "@visulima/packem/css/loader/less";
import sassLoader from "@visulima/packem/css/loader/sass";
import sourceMapLoader from "@visulima/packem/css/loader/sourcemap";
import cssnanoMinifier from "@visulima/packem/css/minifier/cssnano";

export default defineConfig({
transformer,
rollup: {
css: {
mode: "extract",
loaders: [lessLoader, postcssLoader, sassLoader, stylusLoader, sourceMapLoader],
},
},
loaders: [postcssLoader, stylusLoader, lessLoader, sassLoader, sourceMapLoader,],
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

The loader order in the configuration file needs to be updated to match the intended processing sequence

The configuration in examples/styles/packem.config.ts has the loaders in the wrong order. According to the core implementation in packages/packem/src/rollup/plugins/css/index.ts, the intended processing sequence is:

  1. sourcemap
  2. stylus
  3. less
  4. sass
  5. postcss

The current configuration has postcss first and sourcemap last, which contradicts the sorting order enforced by the sortByNameOrder function.

🔗 Analysis chain

Verify CSS loader order for optimal processing.

The order of CSS loaders can impact the processing pipeline. Please verify that this order is intentional:

  1. postcss (for PostCSS processing)
  2. stylus (for Stylus preprocessing)
  3. less (for Less preprocessing)
  4. sass (for Sass preprocessing)
  5. sourcemap (for source map generation)

Consider whether preprocessors (Stylus, Less, Sass) should run before PostCSS for optimal results.


Let me check the actual implementation of the loader ordering logic to understand the intended sequence.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any loader order dependencies or documentation
# that might indicate the preferred processing sequence

# Search for loader order documentation or related tests
rg -g '!node_modules' -g '*.{md,ts,js}' -i 'loader.*order|process.*sequence' 

Length of output: 523


Script:

#!/bin/bash
# Get the implementation of sortByNameOrder and surrounding context
rg -A 10 "sortByNameOrder" packages/packem/src/rollup/plugins/css/index.ts

Length of output: 786

minifier: cssnanoMinifier,
}
}
});
14 changes: 7 additions & 7 deletions packages/packem/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -409,13 +409,13 @@ The value of `type` can be either `"build"` or `"dts"`, where `"build"` is the d

The resolved plugins will be in the following order:

- Alias
- User plugins with enforce: 'pre'
- Rollup core plugins
- User plugins without enforce value
- Rollup build plugins
- User plugins with enforce: 'post'
- Rollup post build plugins (minify, manifest, copy, reporting)
- Alias
- User plugins with enforce: 'pre'
- Rollup core plugins
- User plugins without enforce value
- Rollup build plugins
- User plugins with enforce: 'post'
- Rollup post build plugins (minify, manifest, copy, reporting)

```typescript
import { defineConfig } from "@visulima/packem/config";
Expand Down
2 changes: 1 addition & 1 deletion packages/packem/__tests__/intigration/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ export { a };
await createPackemConfig(temporaryDirectoryPath, {
config: {
onSuccess: "echo hello && echo world",
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the implementation found in packem.ts, I can now generate the final response:

Security and cross-platform concerns confirmed in onSuccess implementation

The current implementation of onSuccess has security vulnerabilities and cross-platform compatibility issues:

  • Commands are executed with shell: true without any sanitization
  • No command validation or allowlist system is implemented
  • The timeout parameter (30s default) is the only safety measure
  • Direct shell command execution may fail on Windows due to shell differences

Recommendations:

  • Add command validation/sanitization before execution
  • Use cross-platform command execution (e.g., cross-spawn)
  • Document security implications in README
  • Add test cases for command injection and failed executions
🔗 Analysis chain

Consider security and cross-platform compatibility in onSuccess tests.

The test cases for the onSuccess command have potential issues:

  1. Security: Executing shell commands directly from config/CLI without validation could be dangerous. Consider adding tests to verify command sanitization.

  2. Cross-platform: The command echo hello && echo world might not work on Windows. Consider using cross-platform alternatives or mocking the command execution.

  3. Test coverage: Add test cases for:

    • Failed commands
    • Command injection attempts
    • Verifying that onSuccess only runs after successful builds

Let's check if there are any command validation or sanitization measures:

Consider:

  1. Using a cross-platform command execution library
  2. Implementing a command allowlist/validation system
  3. Adding documentation about security implications

Let me search for more specific implementation details about the onSuccess command handling.


Let me search for the specific implementation of the onSuccess handler to understand how commands are executed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for command validation in the codebase
rg -A 5 'onSuccess.*command'

Length of output: 1363


Script:

#!/bin/bash
# Search for onSuccess implementation
ast-grep --pattern 'onSuccess'

# Also search for command execution related code
rg -A 10 'exec.*command|spawn.*command'

Length of output: 4627


Script:

#!/bin/bash
# Search for files containing onSuccess implementation
fd -e ts -e js -e mjs onSuccess

# Search for the actual implementation
rg -A 10 "function.*onSuccess|const.*onSuccess"

Length of output: 2312

});

const binProcess = await execPackemSync("build", [], {
Expand Down
171 changes: 164 additions & 7 deletions packages/packem/src/commands/add.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { cwd } from "node:process";

import { installPackage } from "@antfu/install-pkg";
import { confirm, multiselect, select, spinner } from "@clack/prompts";
import type { Cli } from "@visulima/cerebro";
import { readFile, writeFile } from "@visulima/fs";
import { resolve } from "@visulima/path";
import MagicString from "magic-string";

import findPackemFile from "../utils/find-packem-file";
import cssLoaderDependencies from "./utils/css-loader-dependencies";
import type cssMinifierDependencies from "./utils/css-minifier-dependencies";
prisis marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect 'import type' for 'cssMinifierDependencies'

The cssMinifierDependencies is imported using import type, but it's being used as a value in the code (e.g., in typeof cssMinifierDependencies). This will cause runtime errors because import type only brings in types, not actual values.

Apply this diff to fix the import:

-import type cssMinifierDependencies from "./utils/css-minifier-dependencies";
+import cssMinifierDependencies from "./utils/css-minifier-dependencies";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import type cssMinifierDependencies from "./utils/css-minifier-dependencies";
import cssMinifierDependencies from "./utils/css-minifier-dependencies";


const typedocPackages = ["typedoc", "typedoc-plugin-markdown", "typedoc-plugin-rename-defaults", "@ckeditor/typedoc-plugins"];

Expand All @@ -18,12 +21,31 @@ const createAddCommand = (cli: Cli): void => {
required: true,
},
description: "Add a optional packem feature to your project",
// eslint-disable-next-line sonarjs/cognitive-complexity
execute: async ({ argument, logger, options }): Promise<void> => {
const s = spinner();

Comment on lines +25 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure the spinner is properly stopped and cleaned up.

The spinner is started using spinner() from @clack/prompts, but there is no corresponding stop() call to stop the spinner when the command execution is complete or encounters an error. This may lead to the spinner continuing to run indefinitely.

Consider adding a finally block to stop the spinner after the command execution, regardless of whether it succeeds or encounters an error. For example:

try {
  // Command execution logic
} catch (error) {
  // Error handling
} finally {
  s.stop();
}

const rootDirectory = resolve(cwd(), options.dir ?? ".");

const packemConfigFilePath = await findPackemFile(rootDirectory, options.config ?? "");
let packemConfigFilePath: string | undefined;

try {
packemConfigFilePath = await findPackemFile(rootDirectory, options.config);
} catch {
// @TODO: Add a sub command run question to run `packem init` if the user wants to
logger.error("Could not find a packem config file, please run `packem init` first.");

return;
}

const packemConfig: string = await readFile(packemConfigFilePath, { buffer: false });

let packemConfigFormat = "cjs";

if (packemConfig.includes("import")) {
packemConfigFormat = "esm";
}

const magic = new MagicString(packemConfig);

if (argument.includes("typedoc")) {
Expand All @@ -33,11 +55,11 @@ const createAddCommand = (cli: Cli): void => {
return;
}

logger.info("Adding typedoc dependencies...");

await installPackage(typedocPackages, { cwd: process.cwd(), dev: true, silent: true });

magic.prepend(`import typedocBuilder from "@visulima/packem/builder/typedoc";\n`);
if (packemConfigFormat === "cjs") {
magic.prepend(`const typedocBuilder = require("@visulima/packem/builder/typedoc");\n`);
} else {
magic.prepend(`import typedocBuilder from "@visulima/packem/builder/typedoc";\n`);
}

// add the builder key to the packem config, if it doesn't exist
if (packemConfig.includes("builder: {")) {
Expand All @@ -47,7 +69,142 @@ const createAddCommand = (cli: Cli): void => {
magic.replace("transformer,", "transformer,\n builder: {\n typedoc: typedocBuilder,\n },");
}

logger.success("Typedoc added!");
logger.info("Adding typedoc dependencies...");

s.start("Installing packages");
await installPackage(typedocPackages, { cwd: process.cwd(), dev: true, silent: true });
s.stop("Installed packages");

logger.success("\nTypedoc added!");
}

if (argument.includes("css")) {
if (packemConfig.includes("css: {") || packemConfig.includes("@visulima/packem/css")) {
logger.warn("Css loaders have already been added to the packem config.");

return;
}

const cssLoaders: (keyof typeof cssLoaderDependencies | "sourceMap")[] = [];

const mainCssLoader = (await select({
message: "Pick a css loader",
options: [
{ label: "PostCSS", value: "postcss" },
{ label: "Lightning CSS", value: "lightningcss" },
],
})) as keyof typeof cssLoaderDependencies;

cssLoaders.push(mainCssLoader);

let extraCssLoaders = (await multiselect({
message: "Pick extra loaders",
options: [
{ label: "Sass", value: "sass" },
{ label: "Stylus", value: "stylus" },
{ label: "Less", value: "less" },
],
required: false,
})) as (keyof typeof cssLoaderDependencies)[];

if (extraCssLoaders.includes("sass")) {
const sassLoader = await select({
message: "Pick a sass loader",
options: [
{ hint: "recommended", label: "Sass embedded", value: "sass-embedded" },
{ label: "Sass", value: "sass" },
{ hint: "legacy", label: "Node Sass", value: "node-sass" },
],
});

if (sassLoader !== "sass") {
extraCssLoaders = extraCssLoaders.filter((loader) => loader !== "sass");

extraCssLoaders.push(sassLoader as keyof typeof cssLoaderDependencies);
}
}

cssLoaders.push(...extraCssLoaders);

const packagesToInstall: string[] = [];

for (const loader of cssLoaders) {
packagesToInstall.push(...cssLoaderDependencies[loader as keyof typeof cssLoaderDependencies]);
}

cssLoaders.push("sourceMap");

for (let loader of cssLoaders) {
if (loader === "sass-embedded" || loader === "node-sass") {
loader = "sass";
}

if (packemConfigFormat === "cjs") {
magic.prepend(`const ${loader as string}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase() as string}");\n`);
} else {
magic.prepend(`import ${loader as string}Loader from "@visulima/packem/css/loader/${loader.toLowerCase() as string}";\n`);
}
}
Comment on lines +134 to +146
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review inclusion of 'sourceMap' in 'cssLoaders'

Adding "sourceMap" to cssLoaders and treating it as a loader may cause issues, as there may not be a corresponding loader module named sourceMapLoader.

Remove "sourceMap" from cssLoaders and handle the source map configuration separately. This prevents potential runtime errors due to importing a non-existent module.

Apply this diff to fix the issue:

-cssLoaders.push("sourceMap");

 for (let loader of cssLoaders) {
     if (loader === "sass-embedded" || loader === "node-sass") {
         loader = "sass";
     }

-    if (packemConfigFormat === "cjs") {
-        magic.prepend(`const ${loader as string}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase() as string}");\n`);
-    } else {
-        magic.prepend(`import ${loader as string}Loader from "@visulima/packem/css/loader/${loader.toLowerCase() as string}";\n`);
-    }
+    const loaderImport = packemConfigFormat === "cjs"
+        ? `const ${loader}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase()}");\n`
+        : `import ${loader}Loader from "@visulima/packem/css/loader/${loader.toLowerCase()}";\n`;
+
+    magic.prepend(loaderImport);
 }

+// Handle source map configuration separately if required
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cssLoaders.push("sourceMap");
for (let loader of cssLoaders) {
if (loader === "sass-embedded" || loader === "node-sass") {
loader = "sass";
}
if (packemConfigFormat === "cjs") {
magic.prepend(`const ${loader as string}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase() as string}");\n`);
} else {
magic.prepend(`import ${loader as string}Loader from "@visulima/packem/css/loader/${loader.toLowerCase() as string}";\n`);
}
}
for (let loader of cssLoaders) {
if (loader === "sass-embedded" || loader === "node-sass") {
loader = "sass";
}
const loaderImport = packemConfigFormat === "cjs"
? `const ${loader}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase()}");\n`
: `import ${loader}Loader from "@visulima/packem/css/loader/${loader.toLowerCase()}";\n`;
magic.prepend(loaderImport);
}
// Handle source map configuration separately if required


const useCssMinifier = (await confirm({
initialValue: false,
message: "Do you want to minify your css?",
})) as boolean;

let cssMinifier: keyof typeof cssMinifierDependencies | undefined;

if (useCssMinifier) {
cssMinifier = (await select({
message: "Pick a css minifier",
options: [
{ label: "CSSNano", value: "cssnano" },
{ label: "Lightning CSS", value: "lightningcss" },
],
})) as keyof typeof cssMinifierDependencies;

if (!cssLoaders.includes("lightningcss")) {
packagesToInstall.push(cssMinifier);
}

Comment on lines +165 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid adding undefined or duplicate packages

When adding the CSS minifier to packagesToInstall, ensure that the variable cssMinifier is defined and not already included.

Add a check to prevent undefined values and duplicate entries.

 if (cssMinifier && !packagesToInstall.includes(cssMinifier)) {
     packagesToInstall.push(cssMinifier);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
packagesToInstall.push(cssMinifier);
}
if (cssMinifier && !packagesToInstall.includes(cssMinifier)) {
packagesToInstall.push(cssMinifier);
}

if (packemConfigFormat === "cjs") {
magic.prepend(
`const ${cssMinifier as string}Minifier = require("@visulima/packem/css/minifier/${cssMinifier.toLowerCase() as string}");\n`,
);
} else {
magic.prepend(`import ${cssMinifier as string}Minifier from "@visulima/packem/css/minifier/${cssMinifier.toLowerCase() as string}";\n`);
}
}

const stringCssLoaders = cssLoaders
.map((loader) => {
if (loader === "sass-embedded" || loader === "node-sass") {
// eslint-disable-next-line no-param-reassign
loader = "sass";
}

return `${loader}Loader`;
})
.join(", ");

if (packemConfig.includes("rollup: {")) {
magic.replace(
"rollup: {",
`rollup: {\n css: {${cssMinifier ? `\n minifier: ${cssMinifier as string}Minifier,` : ""}\n loaders: [${stringCssLoaders}],\n },\n`,
);
} else {
const transformerPlaceholder = packemConfig.includes(" transformer,") ? " transformer," : " transformer";

magic.replace(
transformerPlaceholder,
` transformer,\n rollup: {\n css: {${cssMinifier ? `\n minifier: ${cssMinifier as string}Minifier,` : ""}\n loaders: [${stringCssLoaders}],\n },\n },`,
);
}

s.start("Installing packages");
await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true });
s.stop("Installed packages");
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle package installation errors gracefully.

While the package installation process is wrapped in a spinner to indicate progress, it's important to handle any errors that may occur during the installation gracefully. If an error occurs, the spinner should be stopped, and an appropriate error message should be logged to inform the user about the failure.

Here's an example of how you can handle package installation errors:

try {
  s.start("Installing packages");
  await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true });
  s.stop("Packages installed successfully");
} catch (error) {
  s.stop("Package installation failed");
  logger.error("An error occurred while installing packages:");
  logger.error(error);
  // Optionally, you can provide instructions on how to manually install the packages
  logger.info("Please try installing the packages manually using the following command:");
  logger.info(`npm install ${packagesToInstall.join(" ")}`);
  return;
}

By catching any errors that occur during package installation, you can provide a better user experience by informing them about the failure and suggesting possible solutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle installation errors gracefully

When installing packages, errors may occur (e.g., network issues, package not found). Currently, the code does not handle such exceptions.

Wrap the package installation in a try-catch block to handle errors gracefully and inform the user.

 try {
     loadingSpinner.start("Installing packages");
     await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true });
     loadingSpinner.stop("Installed packages");
+} catch (error) {
+    loadingSpinner.stop("Failed to install packages");
+    logger.error(`Package installation failed: ${error.message}`);
+    return;
 }

Committable suggestion skipped: line range outside the PR's diff.


logger.success("\nCSS loaders added!");
}

await writeFile(packemConfigFilePath, magic.toString(), {
Expand Down
Loading
Loading