Using webpack with pino for serverside builds

[joeyvandijk]

I understand to make it clear this is only ment not for the browser, but I do not see it being used in other files, so why is this still needed?

With tools like webpack the file is not recognized as a javascript file so it breaks the transpiling when using ES6 code.

Could this be deleted? Then I could use it also with projects that use AWS Lambda, etc.

[mcollina]

I'm lost, what is the problem? You cannot use pino with webpack on the browser? In the browser you should be loading https://github.com/pinojs/pino/blob/master/package.json#L6.

pretty.js is linked here https://github.com/pinojs/pino/blob/master/package.json#L8 as a standalone CLI tool, and you can also use it programmatically if you need to.

[joeyvandijk]

The problem is that webpack (in combination with AWS Lambda) detects pretty.js as a non-js file. I want to ignore it but that is not default behaviour, so I was curious why it needs this line.

Do I understand correctly the proposed change is preventing pretty.js being used as a standalone tool? If so, close this PR, because I need to find another way of preventing webpack to fail recognizing pretty.js as a javascript file and not a command file.

[jsumners]

pretty.js is the "binary" that is executed in myapp | pino. Therefore, it is most definitely needed.

[jsumners]

I think this is a clear example of the benefits of bin.js, @davidmarkclements ;)

pinojs/pino-sapling#1

[davidmarkclements]

Will webpack autoignore bin.js? If so let's rename, if not then it's a general webpack problem

[jsumners]

He's issue, as I understand it, is that we require pretty.js in pino.js. So he can't pack pino.js since his tool doesn't like pretty.js.

[davidmarkclements]

ah I see - still you'd think webpack (like browserify) could handle that...

at any rate the issue here is either webpack isn't recognising the browser field of the package.json or it is but for some other reason it's parsing pino.js and giving the error

webpack should not be touching pino.js at all - it should be reading from browser.js which doesn't require pretty.js

From what I can see in webpack docs it looks for the browser field...

Perhaps it can be solved with

"resolve": {
  "alias": {
    "pino$": "pino/browser.js"
  }
}

In the package.json

@joeyvandijk would you mind giving that a shot and updating PR?

[joeyvandijk]

I will have a look tonight if a rename helps or I can find more information how to make it work with webpack. Thanks for the quick updates and insights.

[joeyvandijk]

@davidmarkclements I am not using Pino in a browser-environment. I am using it for node.js on AWS Lambda (so node.js). So pretty.js is embedded, but webpack skipping the shebang and interpreting pretty.js as a javascript file.

I will test tonight if I can fix it and post it inside this PR.

[davidmarkclements]

so.. you're using webpack to build server code?

[joeyvandijk]

@davidmarkclements yup, to write ES6 node.js code and transpile it back to node.js v4 for AWS Lambda.

[davidmarkclements]

can this help

https://github.com/JavascriptIsMagic/shebang-loader

[joeyvandijk]

@davidmarkclements have tried it, but does not work. Have checked it twice. :/

[joeyvandijk]

@davidmarkclements the whole problem is that pino.js imports pretty.js and that is why it comes into webpack and fails. But if we move the CLI-version in another file, it still can be used with webpack AND the CLI-setup.

Something like

#! /usr/bin/env node

var pretty = require('./pretty.js')
module.exports = pretty

Gonna have a try and run the tests.

[davidmarkclements]

the reason @jsumners gave you thumbsdown there (btw) is because transpiling server side code is quite a risky strategy - you're trusting a lot of layers to rewrite code for you in a trusted environment. I'm sure AWS lambda mitigates some of the security vectors there though...

if you absolute think that's the right approach, the only way I think we can solve this for you is to have a separate bin entry point (bin.js or cmd.js) with a hashbang that requires pretty.js

As long as @mcollina and @jsumners are happy with that solution, I'd be happy to accept if you updated PR accordingly (edit - as you just proposed)

[jsumners]

I think it's a ridiculous situation (transpiling needs to stop), but I'm fine with a bin.js or cmd.js.

[davidmarkclements]

@joeyvandijk - you closed?

[joeyvandijk]

@davidmarkclements apparently I have mistaken git reset to trigger this branch to delete.

The correct code is now in #201 as described above by you and me.

[TheLarkInn]

@jsumners It's actually a performance benefit to bundle server code, not only for faster load, but also faster resolution then nodes native and slow sync resolver. The problem here is that webpack doesn't understand the '#' symbol as valid JS from it being a bin. A loader or alias can be used to ignore this, but I wouldn't dismiss the idea of bundling node code as being not worthwhile, because it very much so is!

[mcollina]

Thanks for checking Pino out @theLarkin!
Do you have any data on those benefits?
It would be fantastic to have webpack strip out the top # line in deps if the goal is to better support Node. There are a lot of modules out there using the same pattern we were using here.

[TheLarkInn]

It raises a great question. Should we treat it just like any other JS and strip the env?

[mcollina]

I think so. Unless it's the entry point, in which case the user is trying to pack an executable. In this case you probably want to add that back in at the top of the file, and probably make some adjustment so that require.main is correctly transformed. This will be great feature if someone wants to ship a cli tool: faster installs, faster execution, etc.. This might require more work. I'm happy to open a issue on webpack and test this feature if someone wants to work on it (I'm currently snowed under, and I can't commit to any more OSS work atm).

[TheLarkInn]

Same here in regards to load but I'll forward this over to our team and discuss. Thanks for the quick response.

…

On Thu, Apr 12, 2018, 6:51 AM Matteo Collina ***@***.***> wrote: I think so. Unless it's the entry point, in which case the user is trying to pack an executable. In this case you probably want to add that back in at the top of the file, and probably make some adjustment so that require.main is correctly transformed. This will be great feature if someone wants to ship a cli tool: faster installs, faster execution, etc.. This might require more work. I'm happy to open a issue on webpack and test this feature if someone wants to work on it (I'm currently snowed under, and I can't commit to any more OSS work atm). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#200 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADQBMAU2tgptGD5z6C5CpcvQWDsMq3Kiks5tn1v2gaJpZM4MUWFy> .

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.