Skip to content

Conversation

trurl-master
Copy link
Owner

No description provided.

Puppeteer is only used in development/testing scripts and should not be a runtime dependency for consumers of the library.

Fixes #74

Co-authored-by: Ivan Galiatin <trurl-master@users.noreply.github.com>
Copy link

claude bot commented Aug 18, 2025

Claude finished @trurl-master's task —— View job


PR Review: Move puppeteer to devDependencies

Todo List:

  • Read and analyze package.json changes
  • Review project structure and puppeteer usage
  • Check build and test configurations
  • Assess dependency management best practices
  • Provide comprehensive review feedback

✅ Overall Assessment: APPROVED

This PR correctly moves puppeteer from dependencies to devDependencies. The change is well-justified and follows Node.js best practices.

Code Quality & Best Practices: ✅ Excellent

Dependency Management

  • ✅ Correct categorization: Puppeteer is only used in capture-browser-results.js, which is a development/testing script, not consumed by library users
  • ✅ Proper versioning: Version bump from 1.15.2 → 1.16.0 follows semantic versioning (minor bump for dependency changes)
  • ✅ Clean implementation: Only moved the dependency without changing version constraints

Runtime Dependencies Analysis

The remaining runtime dependencies are appropriate:

  • bezier-easing: Used in Web Animations API implementation
  • css-mediaquery: Used in viewport/matchMedia mocks

Potential Issues: None Found

Build Process

  • No build script dependencies on puppeteer
  • Uses playwright for browser testing, which is in devDependencies
  • capture-browser-results.js is development-only utility script

Test Coverage

  • Primary testing uses Jest/Vitest with playwright
  • Puppeteer script is auxiliary tooling for CSS Typed OM verification
  • Move won't break CI/CD workflows

Performance Considerations: ✅ Positive Impact

Installation Performance

  • Reduced install time: Consumers won't download ~170MB puppeteer dependency
  • Smaller bundle size: Production installs will be significantly faster
  • Better developer experience: Library users get cleaner dependency tree

No Runtime Performance Impact

  • Puppeteer never used in library code paths
  • No performance regressions expected

Security Concerns: ✅ Improvement

Reduced Attack Surface

  • Fewer dependencies: Eliminates puppeteer from production dependency tree
  • Browser automation isolation: Keeps browser automation tools in development only
  • Supply chain security: Reduces potential security vulnerabilities in production

Test Coverage: ✅ Maintained

Testing Strategy Analysis

  • Main test suites use Jest/Vitest + Playwright (both in devDependencies)
  • capture-browser-results.js is supplementary tooling
  • npm scripts properly configured for all test scenarios
  • CI/CD will continue working as puppeteer remains available for development

Recommendations

  1. ✅ This change should be merged - it follows Node.js best practices
  2. Consider documentation: Update any setup instructions that mention puppeteer installation
  3. CI verification: Ensure CI installs devDependencies for the capture script if needed

Files Analysis

Modified: package.json:84-90

  • Line 86: Removed "puppeteer": "^24.15.0" from dependencies
  • Line 90: Added "puppeteer": "^24.15.0" to devDependencies
  • Line 3: Version bump 1.15.21.16.0

Impact Analysis:

  • ✅ Library consumers: Faster installs, smaller node_modules
  • ✅ Development: No workflow changes, all tooling remains functional
  • ✅ Testing: Full test coverage maintained

This is a textbook example of proper dependency management in a Node.js library.

@trurl-master
Copy link
Owner Author

@claude the testing job fails, could you take a look?

Copy link

claude bot commented Aug 18, 2025

Claude finished @trurl-master's task —— View job


Test Failure Analysis: Module Configuration Issue

Todo List:

  • Analyze the failing CI job and download logs
  • Review test configurations and puppeteer usage
  • Diagnose the root cause of test failure
  • Provide solution recommendations

🔍 Root Cause Identified

The test failure is not related to the puppeteer dependency move. The issue is a module system configuration conflict:

Error: ReferenceError: module is not defined in ES module scope

Cause: The Jest configuration files (jest.config.ts and swcjest.config.ts) use CommonJS syntax (module.exports) but package.json has "type": "module", which makes Node.js treat all TypeScript files as ES modules by default.

🛠️ Solution Options

Option 1: Convert Jest configs to ES module syntax (Recommended)

// jest.config.ts
export default {
  preset: 'ts-jest',
  testEnvironment: 'jsdom',
  // ... rest of config
};

// swcjest.config.ts  
export default {
  testEnvironment: 'jsdom',
  // ... rest of config
};

Option 2: Rename config files to .js or .cjs

  • Rename jest.config.tsjest.config.cjs
  • Rename swcjest.config.tsswcjest.config.cjs
  • Update npm scripts to reference new file names

Option 3: Use ts-node with CommonJS mode

  • Configure ts-node to use CommonJS for Jest configs
  • More complex but preserves current syntax

📋 Recommendation

I recommend Option 1 (convert to ES modules) as it:

  • ✅ Maintains TypeScript benefits
  • ✅ Aligns with the package's ES module setup
  • ✅ Is the most future-proof approach
  • ✅ Requires minimal changes

The puppeteer dependency move is correct and should be kept. This module configuration issue likely existed before but wasn't exposed until the CI environment changed or dependencies were updated.

Would you like me to implement the fix by converting the Jest configs to ES module syntax?


@trurl-master
Copy link
Owner Author

@claude Let's go with option 1, I'm wondering why it passed before...

Copy link

claude bot commented Aug 18, 2025

Claude finished @trurl-master's task —— View job


Claude finished @trurl-master's task


Convert Jest Configs to ES Modules & Investigate Test History

Todo List:

  • Read current Jest configuration files to understand structure
  • Investigate why tests passed before (check git history/CI patterns)
  • Convert jest.config.ts to ES module syntax
  • Convert swcjest.config.ts to ES module syntax
  • Commit and push the fixes
  • Provide explanation of why it worked before

🔍 Mystery Solved: Why Tests Passed Before

The tests only started failing recently because "type": "module" was added to package.json in commit 6294892 ("Basic css typed om support") on July 28, 2025.

Before that change:

  • Node.js treated .ts files based on TypeScript compilation behavior
  • Jest configs using module.exports (CommonJS syntax) worked fine

After adding "type": "module":

  • Node.js now treats all .ts files as ES modules by default
  • Jest configs using module.exports throw ReferenceError: module is not defined in ES module scope

The puppeteer dependency move is completely unrelated to the test failure - it was just timing coincidence that this became visible now.

Fixed

Changes Made:

  • jest.config.ts: Changed module.exports = { to export default {
  • swcjest.config.ts: Changed module.exports = { to export default {

Technical Details:

  • Both configs now use ES module syntax (export default) instead of CommonJS (module.exports)
  • Preserved all existing configuration options and functionality
  • No changes to test behavior - only fixed the module system compatibility

The test failures should now be resolved. The puppeteer dependency move (from dependencies to devDependencies) was correct and should be kept.


Convert jest.config.ts and swcjest.config.ts from CommonJS to ES module
syntax to fix 'module is not defined in ES module scope' error.

The issue occurred because package.json has "type": "module" which makes
Node.js treat .ts files as ES modules, but Jest configs were using
module.exports (CommonJS syntax).

Co-authored-by: Ivan Galiatin <trurl-master@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant