Test files included in release tarball and `module.exports` based on content of directory are messing with `esbuild` bundler

[marvinruder]

Release version 10.2.2 contains the following code which dynamically processes all JavaScript files in the same directory, supposedly excluding test files:

https://github.com/pinojs/pino-pretty/blob/9fb73303d48162e3332f0305143e159f98566d10/lib/utils/index.js#L6-L17

However, not all bundlers can handle this code. ESBuild for example describes their capability of bundling glob-style imports here. Since

Each non-string expression in the string concatenation chain becomes a wildcard in a glob pattern

it is unable to detect that *.test.js files are not to be bundled.

This leads to errors such as:

✘ [ERROR] Could not resolve "tap"

    node_modules/pino-pretty/lib/utils/join-lines-with-indentation.test.js:3:20:
      3 │ const tap = require('tap')
        ╵                     ~~~~~

This is easily reproducable using the following minimal setup:

`package.json`: ```json { "main": "index.js", "scripts": { "esbuild": "esbuild index.js --bundle --platform=node --outfile=bundle.js" }, "dependencies": { "esbuild": "0.19.4", "pino-pretty": "10.2.2" } } ``` `index.js`: ```js const pinoPretty = require('pino-pretty'); ``` Running `find node_modules/pino-pretty -name "*.test.js" -delete` solves the issue.

Would it be possible to not ship .test.js files with pino-pretty, e.g. by removing them as part your build and release pipeline?

[gaeduron]

I have the same issue I temporarily fixed it by adding tap to my package by json. with : npm i tap

[c0mm4nd]

Same issue.

On nextjs:

Module not found: Can't resolve 'tap'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/pino-pretty/lib/utils/ sync ^\.\/.*\.js$
./node_modules/pino-pretty/lib/utils/index.js
./node_modules/pino-pretty/index.js
./node_modules/pino/lib/tools.js
./node_modules/pino/pino.js
./node_modules/@walletconnect/logger/dist/cjs/index.js
./node_modules/@walletconnect/universal-provider/dist/index.es.js
./node_modules/@walletconnect/ethereum-provider/dist/index.es.js
./node_modules/@wagmi/connectors/dist/walletConnect.js
./node_modules/@wagmi/core/dist/connectors/walletConnect.js
./node_modules/wagmi/dist/connectors/walletConnect.js
./node_modules/@rainbow-me/rainbowkit/dist/index.js
./app/page.tsx

Try fix with npm i tap, still errors:

./node_modules/@tapjs/stack/dist/commonjs/require-resolve.js
Critical dependency: require function is used in a way in which dependencies cannot be statically extracted

Import trace for requested module:
./node_modules/@tapjs/stack/dist/commonjs/require-resolve.js
./node_modules/@tapjs/stack/dist/commonjs/index.js
./node_modules/@tapjs/core/dist/commonjs/extra-from-error.js
./node_modules/@tapjs/core/dist/commonjs/index.js
./node_modules/tap/dist/commonjs/main.js
./node_modules/tap/dist/commonjs/index.js
./node_modules/pino-pretty/lib/utils/build-safe-sonic-boom.test.js
./node_modules/pino-pretty/lib/utils/ sync ^\.\/.*\.js$
./node_modules/pino-pretty/lib/utils/index.js
./node_modules/pino-pretty/index.js
./node_modules/pino/lib/tools.js
./node_modules/pino/pino.js
./node_modules/@walletconnect/logger/dist/cjs/index.js
./node_modules/@walletconnect/universal-provider/dist/index.es.js
./node_modules/@walletconnect/ethereum-provider/dist/index.es.js
./node_modules/@wagmi/connectors/dist/walletConnect.js
./node_modules/@wagmi/core/dist/connectors/walletConnect.js
./node_modules/wagmi/dist/connectors/walletConnect.js
./node_modules/@rainbow-me/rainbowkit/dist/index.js
./app/page.tsx

[jsumners]

In my opinion, this is a problem with the tooling you are using. This module is intended to be used within a Node.js environment. Such an environment should have no issue with the code in question.

[mcollina]

@jsumners I think we should refactor that block, mostly because quite a few people bundle their server dependencies and it would not work there. It also loads slightly slower than just listing all files manually.

[jsumners]

I don't care for that approach because it requires ongoing maintenance. But the index.js was added to remove the need to search through the project to find and update blocks like: https://github.com/pinojs/pino-pretty/blob/9fb73303d48162e3332f0305143e159f98566d10/index.js#L15-L18

I won't block a PR that solve the issue either way.

[marvinruder]

Considering that I do not see any complex tool configurations in your repository, I assume you are creating the npm tarball by running npm pack, so a simple .npmignore file should do the trick here. Something like

# Test files
*.test.js
test/

# Example images
*.png

# Other files only used in development
.*
benchmark.js
coverage-map.js
tsconfig.json

will not only exclude test files, but also some other development-only files, from being published to npm. This will also reduce the tarball size:

# Before:
npm notice === Tarball Details === 
npm notice name:          pino-pretty                             
npm notice version:       10.2.2                                  
npm notice filename:      pino-pretty-10.2.2.tgz                  
npm notice package size:  59.9 kB                                 
npm notice unpacked size: 217.7 kB                                
npm notice shasum:        d0866e88d8a269648f8c4204c1efcc7d5350c78d
npm notice integrity:     sha512-gIjPQ7JqnJ1UT[...]F//EiU+2spZEQ==
npm notice total files:   73                                      

# After
npm notice === Tarball Details === 
npm notice name:          pino-pretty                             
npm notice version:       10.2.2                                  
npm notice filename:      pino-pretty-10.2.2.tgz                  
npm notice package size:  24.5 kB                                 
npm notice unpacked size: 81.0 kB                                 
npm notice shasum:        2b93ca8536b5fd1f53c377f941e3269afe1ed659
npm notice integrity:     sha512-18PUK/b9b0cCQ[...]Zb/rd36lxwLrw==
npm notice total files:   34                                      

Let me know what you think. I can create a PR with that change later today.

[jsumners]

Please see the discussion in https://github.com/fastify/skeleton/issues/42#issuecomment-1208090783 for my reasoning against .npmignore.

The changes that will solve this are either of:

1. Update lib/index.js to manually require all lib scripts and add them to module.exports.
2. Update the various parts of the code base that rely on require('./lib/') to instead require the individual lib scripts needed and remove lib/index.js completely.

[marvinruder]

Please see the discussion in fastify/skeleton#42 (comment) for my reasoning against .npmignore.

Understandable. One could add some fancy pack shortcut to the package.json’s scripts section like cp .gitignore .npmignore && cat .npmignoretemplate >> .npmignore && npm pack; rm -f .npmignore to solve the problem of maintaining two different files, but I understand that this may be considered a very hacky and questionable solution.

1. Update lib/index.js to manually require all lib scripts and add them to module.exports.

I can implement this later.