Fix bug with 2 separate PDFJS objects, add more Node/Bun tests in CI
#384
Conversation
|
@Electroid I'm a big fan of your work! Unfortunately I cannot take action on this PR, but the code looks good! Also, keeping support for older versions of NodeJs is always a plus. |
modesty
left a comment
modesty
left a comment
There was a problem hiding this comment.
thanks for extending the tests and runtime, appreciated.
better to keep bun related info as extension, not replacement. two reasons:
- the package should work with or without
bun - existing devOp automation is using
npmcommands, shoud not break
| "test": "jest --config ./jest.config.json && npm run parse-r && npm run parse-fd", | ||
| "pretest:node": "npm run build", | ||
| "test:node": "jest && npm run parse-r && npm run parse-fd", | ||
| "pretest:bun": "npm run build", |
There was a problem hiding this comment.
what's the reason to add pretest:bun while unused and same as existing pretest ?
better leave pretest, test as before, and add xxx:bun, so that it'd won't break existing devop automation.
| `pretest` step builds bundles and source maps for both ES Module and CommonJS, output to `./dist` directory. The Jest test suit is defined in `./test/_test_.cjs` with commonJS, test run will also cover `parse-r` and `parse-fd` with ES Modules via command line. | ||
| ```bash | ||
| bun run test:bun # runs in Bun | ||
| bun run test:node # runs in Node.js using Jest |
There was a problem hiding this comment.
better keep npm test as to not breaking existing devop automation.
keep the bun related info as extension, not replacement.
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| node-version: [18, 20, 22] |
There was a problem hiding this comment.
node@v18 support is deprecated now
| with: | ||
| node-version: ${{ matrix.node-version }} | ||
|
|
||
| - name: Setup Bun |
There was a problem hiding this comment.
why need to Setup Bun in node job?
| > npm update pdf2json -g | ||
| ## Usage | ||
|
|
||
| To Run in RESTful Web Service or as command line Utility |
There was a problem hiding this comment.
one of the key feature of pdf2json is to run in a service, this document pointer needs to stay
| ``` | ||
|
|
||
| ## Test | ||
| The module is tested with [Node.js](https://nodejs.org/) 18+ and [Bun](https://bun.sh/) 1+. |
There was a problem hiding this comment.
the lowest supported version of node is 20.x
| > npm test | ||
| You can run tests in Bun, or in Node.js using Jest. | ||
|
|
||
| `pretest` step builds bundles and source maps for both ES Module and CommonJS, output to `./dist` directory. The Jest test suit is defined in `./test/_test_.cjs` with commonJS, test run will also cover `parse-r` and `parse-fd` with ES Modules via command line. |
There was a problem hiding this comment.
pretest is important to get build and test works, it needs to stay
There was a problem hiding this comment.
jest.config.json needs to stay, it makes default node env and matching file name convension explicit.
3 participants
This PR fixes a bug where there were 2 separate
PDFJSobjects, and adds support to run tests in Bun.Electroid@69ecd47 fixes the
PDFJSbugElectroid@f516266 & Electroid@c5725b5 make changes so Node 18.x, 20.x, 22.x and Bun 1.x are tested in CI
Thanks for working on this package and happy to help!