-
Notifications
You must be signed in to change notification settings - Fork 254
Add npm package build and publish flow #36
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
Conversation
…dd-github-actions-for-npm-package
There was a problem hiding this 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
.vscodeignoreto accommodatepdfjs-distandcanvas - Introduced a new
pdf.tsutility for PDF parsing viapdfjs-distand updated document rendering - Added
packages/poml-buildsetup (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'; | |||
Copilot
AI
Jul 8, 2025
There was a problem hiding this comment.
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.
| const rootPackages = ['sharp', 'pdf-parse']; | ||
| // For example: | ||
| // const rootPackages = ['sharp', 'pdf-parse', 'pdfjs-dist']; | ||
| const rootPackages = ['sharp']; |
Copilot
AI
Jul 8, 2025
There was a problem hiding this comment.
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.
| const rootPackages = ['sharp']; | |
| const rootPackages = ['sharp', 'pdfjs-dist', 'canvas']; |
| }); | ||
|
|
||
| test('emptyMessages', () => { | ||
| // Turn off console.warn in this test case. |
Copilot
AI
Jul 8, 2025
There was a problem hiding this comment.
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.
| } = require('lodash'); // eslint-disable-line | ||
| const { BaseXmlCstVisitor } = require('@xml-tools/parser'); // eslint-disable-line | ||
| const { findNextTextualToken } = require('@xml-tools/common'); // eslint-disable-line |
Copilot
AI
Jul 8, 2025
There was a problem hiding this comment.
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.
| } = 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 |
| } | ||
|
|
||
| interface FreeOptions { } | ||
| type FreeOptions = any; |
Copilot
AI
Jul 8, 2025
There was a problem hiding this comment.
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.
| type FreeOptions = any; | |
| interface FreeOptions {} |
This pull request introduces significant changes to enhance the functionality of the
pomllibrary, improve dependency management, and replace thepdf-parselibrary withpdfjs-distfor PDF handling. Additionally, it includes updates to build configurations and test cases to ensure compatibility and maintainability.New Features and Enhancements:
pomlnpm package: Created a new npm package for thePrompt 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:
pdf-parsewithpdfjs-dist: Migrated frompdf-parsetopdfjs-distfor PDF parsing, adding utility functionspdfParseandgetNumPagesinpackages/poml/util/pdf.ts. Updated related imports and logic indocument.tsxto use the new library. (packages/poml/util/pdf.ts,packages/poml/components/document.tsx) [1] [2]package.jsondependencies: Added new dependencies like@rollup/plugin-commonjs,@rollup/plugin-json, andpdfjs-dist. Upgraded several dev dependencies, including@typescript-eslintandtypescript. (package.json) [1] [2]Build and Configuration Changes:
.github/workflows/publish.ymlto build, pack, and publish thepomlnpm package on Ubuntu. (.github/workflows/publish.yml).vscodeignoreupdates: Adjusted configurations to includepdfjs-distand excludepdf-parsein the build and packaging process. (webpack.config.cli.js,webpack.config.extension.js,vscodeignore.js) [1] [2] [3]Codebase Refactoring and Testing:
typealiases for flexibility and cleaned up unnecessary ESLint comments. (packages/poml/writer.ts,packages/poml/util/xmlContentAssist.js) [1] [2] [3]packages/poml/tests/util.test.tsx,packages/poml/tests/writer.test.tsx) [1] [2] [3]## Summarypackages/pomlpublish.ymlworkflow to pack and publish the npm packageTesting
npm run build-webviewnpm run build-clinpm run lintnpm testpython -m pytest python/testshttps://chatgpt.com/codex/tasks/task_e_686378fcbccc832e982df1c701717dbd