Skip to content

Conversation

@ultmaster
Copy link
Contributor

@ultmaster ultmaster commented Jul 1, 2025

This pull request introduces significant changes to enhance the functionality of the poml library, improve dependency management, and replace the pdf-parse library with pdfjs-dist for PDF handling. Additionally, it includes updates to build configurations and test cases to ensure compatibility and maintainability.

New Features and Enhancements:

  • Added poml npm package: Created a new npm package for the Prompt Orchestration Markup Language (POML), including its build scripts, dependencies, and CLI support. (packages/poml-build/package.json, packages/poml-build/rollup.confg.js, packages/poml-build/tsconfig.json, packages/poml/cli.ts) [1] [2] [3] [4]

Dependency and Library Updates:

  • Replaced pdf-parse with pdfjs-dist: Migrated from pdf-parse to pdfjs-dist for PDF parsing, adding utility functions pdfParse and getNumPages in packages/poml/util/pdf.ts. Updated related imports and logic in document.tsx to use the new library. (packages/poml/util/pdf.ts, packages/poml/components/document.tsx) [1] [2]
  • Updated package.json dependencies: Added new dependencies like @rollup/plugin-commonjs, @rollup/plugin-json, and pdfjs-dist. Upgraded several dev dependencies, including @typescript-eslint and typescript. (package.json) [1] [2]

Build and Configuration Changes:

  • Updated workflows for npm package publishing: Modified .github/workflows/publish.yml to build, pack, and publish the poml npm package on Ubuntu. (.github/workflows/publish.yml)
  • Webpack and .vscodeignore updates: Adjusted configurations to include pdfjs-dist and exclude pdf-parse in the build and packaging process. (webpack.config.cli.js, webpack.config.extension.js, vscodeignore.js) [1] [2] [3]

Codebase Refactoring and Testing:

  • Refactored type definitions and imports: Replaced interfaces with type aliases for flexibility and cleaned up unnecessary ESLint comments. (packages/poml/writer.ts, packages/poml/util/xmlContentAssist.js) [1] [2] [3]
  • Test improvements: Enhanced test cases by suppressing irrelevant warnings and updating assertions for better reliability. (packages/poml/tests/util.test.tsx, packages/poml/tests/writer.test.tsx) [1] [2] [3]## Summary
  • add npm package configuration for packages/poml
  • provide tsconfig for building the package
  • add npm scripts for building, packing and publishing the library
  • extend publish.yml workflow to pack and publish the npm package

Testing

  • npm run build-webview
  • npm run build-cli
  • npm run lint
  • npm test
  • python -m pytest python/tests

https://chatgpt.com/codex/tasks/task_e_686378fcbccc832e982df1c701717dbd

@ultmaster ultmaster added the v0.1 label Jul 3, 2025
@ultmaster ultmaster marked this pull request as ready for review July 8, 2025 07:28
Copilot AI review requested due to automatic review settings July 8, 2025 07:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds build and publish flow for the poml npm package, including configuration, build scripts, and CI workflow extensions

  • Extended webpack configs and .vscodeignore to accommodate pdfjs-dist and canvas
  • Introduced a new pdf.ts utility for PDF parsing via pdfjs-dist and updated document rendering
  • Added packages/poml-build setup (tsconfig, Rollup config, package.json) and CI steps to pack/publish the library

Reviewed Changes

Copilot reviewed 18 out of 22 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
webpack.config.extension.js Added pdfjs-dist and canvas to externals
webpack.config.cli.js Added pdfjs-dist and canvas externals to CLI build
vscodeignore.js Updated .vscodeignore generation and rootPackages comment
packages/poml/writer.ts Changed empty option interfaces to any types
packages/poml/util/xmlContentAssist.js Simplified ESLint disable comments
packages/poml/util/pdf.ts New PDF parsing utility using pdfjs-dist
packages/poml/components/document.tsx Switched from pdf-parse to pdfParse and getNumPages
packages/poml/tests/writer.test.tsx Overrode and restored console.warn in one test
packages/poml/tests/util.test.tsx Broadened test matcher for loading output
packages/poml/index.ts Exported types separately
packages/poml/cli.ts Added Node.js shebang for CLI
packages/poml-build/tsconfig.json New TS config for building the package
packages/poml-build/rollup.confg.js Rollup setup for ESM/CJS outputs
packages/poml-build/package.json npm metadata, scripts, dependencies for publishing poml
package.json (root) Updated devDependencies and overrides
.vscodeignore (root) Updated to include pdfjs-dist
.github/workflows/publish.yml Added CI steps to pack and publish the npm package
Comments suppressed due to low confidence (2)

.github/workflows/publish.yml:127

  • The 'pack-poml-lib' script isn't defined in the root package.json; either define this script or adjust the workflow to use the correct packaging command/path.
      run: npm run pack-poml-lib

packages/poml-build/rollup.confg.js:20

  • The comment says 'Turn off source maps for production' but sourcemap is enabled; update the comment to reflect the actual behavior or disable source maps if that was intended.
      sourcemap: true, // Turn off source maps for production

@@ -0,0 +1,38 @@
import commonjs from '@rollup/plugin-commonjs';
Copy link

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

Filename 'rollup.confg.js' has a typo; it should be 'rollup.config.js' so Rollup can locate and use this configuration.

Copilot uses AI. Check for mistakes.
const rootPackages = ['sharp', 'pdf-parse'];
// For example:
// const rootPackages = ['sharp', 'pdf-parse', 'pdfjs-dist'];
const rootPackages = ['sharp'];
Copy link

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

[nitpick] Since you've added pdfjs-dist and canvas as externals, consider including them in rootPackages so they aren't inadvertently ignored during shipping.

Suggested change
const rootPackages = ['sharp'];
const rootPackages = ['sharp', 'pdfjs-dist', 'canvas'];

Copilot uses AI. Check for mistakes.
});

test('emptyMessages', () => {
// Turn off console.warn in this test case.
Copy link

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using a try/finally block or Jest's setup/teardown hooks to ensure console.warn is always restored, even if the test throws an error.

Copilot uses AI. Check for mistakes.
Comment on lines +13 to +15
} = require('lodash'); // eslint-disable-line
const { BaseXmlCstVisitor } = require('@xml-tools/parser'); // eslint-disable-line
const { findNextTextualToken } = require('@xml-tools/common'); // eslint-disable-line
Copy link

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

Using a generic eslint-disable-line can suppress unrelated warnings; consider specifying the exact rule(s) (e.g., @typescript-eslint/no-var-requires) to disable.

Suggested change
} = require('lodash'); // eslint-disable-line
const { BaseXmlCstVisitor } = require('@xml-tools/parser'); // eslint-disable-line
const { findNextTextualToken } = require('@xml-tools/common'); // eslint-disable-line
} = require('lodash'); // eslint-disable-line @typescript-eslint/no-var-requires
const { BaseXmlCstVisitor } = require('@xml-tools/parser'); // eslint-disable-line @typescript-eslint/no-var-requires
const { findNextTextualToken } = require('@xml-tools/common'); // eslint-disable-line @typescript-eslint/no-var-requires

Copilot uses AI. Check for mistakes.
}

interface FreeOptions { }
type FreeOptions = any;
Copy link

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

[nitpick] Defining FreeOptions as any disables type safety; consider creating a more specific interface or extending an existing type to improve maintainability.

Suggested change
type FreeOptions = any;
interface FreeOptions {}

Copilot uses AI. Check for mistakes.
@ultmaster ultmaster merged commit 7ef3e14 into main Jul 8, 2025
4 checks passed
@ultmaster ultmaster deleted the codex/add-github-actions-for-npm-package branch August 27, 2025 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants