MacFJA / js-pino-fingers-crossed

A pino transport hold log until a log level is reach
MIT License
1 stars 1 forks source link

Signature mismatch between types and CJS default export #1

Open flo-sch opened 1 month ago

flo-sch commented 1 month ago

Hello,

Thank you for this package, I see a great value in it, to reduce the noise and not even mentionning how much logs can cost in some cloud environments.

I am trying to use it in a node.js application (typescript) bundled with esbuild, but it seems the generated types and exports (CJS) do not match. NOTE: I use a monorepository (NX) which does not support ESM as of now.

When I try to use it as shown in the documentation, TypeScript is happy:

import fingersCrossed, { enable } from "@macfja/pino-fingers-crossed";
console.log('fingersCrossed', typeof fingersCrossed, fingersCrossed);

const loggerInstance = pino(fingersCrossed(undefined));
loggerInstance.setBindings({ [enable]: bindingLevel });

The console show the following:

fingersCrossed object { default: [Function: index], enable: '#_fingers_crossed' }

Which makes sense according to the documentation of rollup's named exports, and the content of the exported index.cjs:

// index.cjs
function index ( ... )

exports.default = index;
exports.enable = enable;

But the application crashes at runtime with the following error:

TypeError: (0 , import_pino_fingers_crossed.default) is not a function

Or when trying to call fingersCrossed.default() instead of fingersCrossed():

Property 'default' does not exist on type '(transport: any) => DestinationStreamWithMetadata & PassThroughStream'.

May I suggest to export the default function also as a named export? While it would not fix the mismatch (and I do not know rollup enough to figure out how to achieve this), it would allow to use named exports with a matching signature at least.

// index.mjs
export function fingersCrossed (transport) {
  // ...
}
export default fingersCrossed;
import { enable, fingersCrossed } from "@macfja/pino-fingers-crossed";
flo-sch commented 1 month ago

Happy to submit a PR with the named export, by the way?

MacFJA commented 1 month ago

My bad!

I forgot to update the documentation for CJS. It did write it on the getpino website (here) but didn't put here :confused:

Here what it looks like:

const pino = require('pino');
const { default: fingersCrossed, enable } = require('@macfja/pino-fingers-crossed')

const logger = pino(fingersCrossed());

logger.info('Will appear immedialty')
logger.error('Will appear immedialty')

logger.setBindings({ [enable]: 50 })
logger.info('Will NOT appear immedialty')
logger.info('Will NOT appear immedialty')
logger.error('Will appear immedialty as well as the 2 previous messages') // error log are level 50
logger.info('Will NOT appear')
logger.info({ [enable]: false }, 'Will appear immedialty')
logger.info('Will NOT appear')

I tried to make it the same for ESM and CJS, but Rollup can't export both default and named for CJS.

If you can achieve that without the need to have to write the src/index.cjs and src/index.mjs (so one is converted to the other) it would be great

flo-sch commented 1 month ago

I tried to make it the same for ESM and CJS, but Rollup can't export both default and named for CJS

Yes I came to the same conclusion.

Actually I am using typescript, so I use import syntax instead of require but it targets the CJS file (guessing because my monorepo is not converted to a ESM module)

If you can achieve that without the need to have to write the src/index.cjs and src/index.mjs

I tried using that plugin: https://github.com/mnrendra/rollup-plugin-mixexport

It essentially adds the following to the compiled index.cjs file:

// ...
module.exports = exports.default;
Object.defineProperties(module.exports, {
  __esModule: { value: true },
  enable: { value: exports.enable },
  default: { value: exports.default }
});

Which seems to do the trick in my context, I tried using a pnpm patch with that and can confirm that the default import works