pinojs / pino-webpack-plugin

MIT License
19 stars 9 forks source link

Using transport with file with Next.js #81

Open GodDrinkTeJAVA opened 1 year ago

GodDrinkTeJAVA commented 1 year ago

While importing PinoWebpackPlugin in Next.config.js, next.js always shows the message for not having CommonJsRequireDependency.

When I push PinoWebpackPlugin into array of plugins by this clause where currDir is simply a variable for __dirname

config.plugins.push(new PinoWebpackPlugin({transports: [`${currDir}/src/modules/logger/transport.mjs`]}));

The error for dependency always occurs, regardless of my TS-baed project, or new JS-based project created by create-next-app. next build always fail, and dev-server throws error for it.

The message below is shown by error page:

Failed to compile
./node_modules/pino/browser.js
No template for dependency: CommonJsRequireDependency

The message below is shown by terminal running server:

No template for dependency: CommonJsRequireDependency
{"level":30,"time":1673234383826,"pid":23900,"hostname":"<MyDevice>.local","msg":"info logger"} # this is the log I've created
Could not find files for / in .next/build-manifest.json
Could not find files for / in .next/build-manifest.json
<w> [webpack.cache.PackFileCacheStrategy] Skipped not serializable cache item 'Compilation/modules|javascript/dynamic|/Users/user/workspace/project/test/next-test/node_modules/pino/browser.js': No serializer registered for CommonJsRequireDependency
<w> while serializing webpack/lib/cache/PackFileCacheStrategy.PackContentItems -> webpack/lib/NormalModule -> Array { 3 items } -> CommonJsRequireDependency

I've searched for the CommonJsRequireDependency error cases and haven't found any clue for it. If further information is needed, let me know by leaving a comment for that.

mcollina commented 1 year ago

@ShogunPanda wdyt?

ShogunPanda commented 1 year ago

I need to look into it.

@GodDrinkTeJAVA Can you please provide me a reproducible repository or a list of commands I can execute to reproduce this?

GodDrinkTeJAVA commented 1 year ago

https://github.com/GodDrinkTeJAVA/pino-webpack-test

This is a simple example project made from create-next-app. In modules/logger.ts, Pino constructor actually has no transport property, and only adding PinoWebpackPlugin to plugin arrays from next.config.js causes the error. dev-server shows error information page when accessed, and next build fails with same error.

If any of my implementation is broken, I would appreciate a lot for letting me notified.

ShogunPanda commented 1 year ago

I tried cloning your repo and then run next build or next dev and never got any issue. Have you tried deleting node_modules files and any lockfile, reinstall from scratch and build with Next again?

GodDrinkTeJAVA commented 1 year ago

I've checked out the commit and you were right. I'll check more conditions and re-comment when the problem is correctly found. Oh.. and.. could you tell me which directory should be given to transport.target when target is js file? I supposed it be moved to the root so it would be like '__dirname/{filename}' no matter where the file is. Would this be right?

ShogunPanda commented 1 year ago

I'm not sure I understand the question. Can you please clarify?

GodDrinkTeJAVA commented 1 year ago

Sorry if it confuses you. What I just tried to tell was to check out which way of directory should be given as transport.target property in pino() function call.

pino({
    transport: {
        target: PATH_TO_TRANSPORT
    }
})

README.md says each file specified in plugin constructor's transports option would be generated at the root of the dist folder. I thought file with pino() function call would be generated in top level, too. Sorry for confusing comment.

ShogunPanda commented 1 year ago

@mcollina I think you have more knowledge in this. Can you help?

GodDrinkTeJAVA commented 1 year ago

https://github.com/GodDrinkTeJAVA/pino-webpack-test

Also, I found the way to reproduce the error. Now main branch shows error when npm build. Commenting out lines which push PinoWebpackPlugin will fix the error, but when included back, it gives 'No template for dependency: CommonJsRequireDependency' error again.

ristomatti commented 1 year ago

Sorry about the somewhat off topic comment but I fail to understand what benefit using this would bring to a Next.js app? We're currently using Pino 7 with Next.js like any other library and everything seems to work as expected.

mcollina commented 1 year ago

Sorry about the somewhat off topic comment but I fail to understand what benefit using this would bring to a Next.js app? We're currently using Pino 7 with Next.js like any other library and everything seems to work as expected.

This plugin allows you to use transports in a bundled server component. Not something that I would recommend, but it seems people are really interested in doing it.

Xevion commented 1 year ago

Has anyone here found a way forward for using Next.js with Pino transports? I've searched everywhere and come to no conclusion. #109 is the exact same issue as me and has received no attention from the PinoJS community in months.

What do Next.js developers do, as there doesn't seem to be any other great logging libraries out there...

mcollina commented 1 year ago

https://github.com/pinojs/pino-webpack-plugin/issues/109 is the exact same issue as me and has received no attention from the PinoJS community in months.

Pull Requests are welcomed, especially for bugs like this. Unfortunately it does not help that none of us are using Next.js :(.

groozin commented 11 months ago

Hopefully the below helps in some following work on this, as I could not fix this problem.

  1. When next compiles it runs webpack with 2 targets: web and node. For node the pino module is pulled but for web it substitutes it with ./browser.js (as indicated in the pinos package.json in the browser prop). This means that if we include logger.info("asdf") as part of the client code the webpack will try and pull in the ./browser.js. And this somehow conflicts with this plugin.

  2. I am not 100% sure what we are trying to achieve here, by using the same logger configuration on both client and server, but there are some bullet points worth mentioning:

    • pino itself exposes browser compatible api that can be used to log on the client - this however means we need to use it differently than on the server see https://github.com/pinojs/pino/blob/master/docs/browser.md

    • not all transport plugins will work on the client - as some of them might use only node side modules

    • even though we include transports in the plugin configuration - we still need to configure the logger with them to work, so something like this might be necessary. Also the __dirname will be resolved to the path in the .next build folder - I think

import pino from "pino";

const logger = pino({
  transport: {
    target: `${__dirname}/modules/logger/transport.mjs`,
  },
});

export default logger;

Hope this helps for further investigation.

magnusriga commented 3 weeks ago

Sorry about the somewhat off topic comment but I fail to understand what benefit using this would bring to a Next.js app? We're currently using Pino 7 with Next.js like any other library and everything seems to work as expected.

This plugin allows you to use transports in a bundled server component. Not something that I would recommend, but it seems people are really interested in doing it.

Hi @mcollina . Without transports it is not possible to save the log to a file or push the log to an external SaaS log tracker. WIthout transports, in Next.js, pino can only be used without transport, or with pino-pretty if it is passed in as a stream. In other words, without bundling the worker etc, it is not possible to use pino in a meaningful way in Next.js 14 apps.

Am I missing something, or would you agree?

magnusriga commented 3 weeks ago

Sorry about the somewhat off topic comment but I fail to understand what benefit using this would bring to a Next.js app? We're currently using Pino 7 with Next.js like any other library and everything seems to work as expected.

@ristomatti Are you doing it with transports? It does not work on my end in Next.js 14. Without transports, pino becomes a lot less useful (i.e. only console printing, unless I am missing something).