-
Notifications
You must be signed in to change notification settings - Fork 848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using webpack with pino for serverside builds #200
Conversation
I'm lost, what is the problem? You cannot use
|
The problem is that webpack (in combination with AWS Lambda) detects 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 |
|
I think this is a clear example of the benefits of |
Will webpack autoignore bin.js? If so let's rename, if not then it's a general webpack problem |
He's issue, as I understand it, is that we require |
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 webpack should not be touching From what I can see in webpack docs it looks for the browser field... Perhaps it can be solved with
In the @joeyvandijk would you mind giving that a shot and updating PR? |
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. |
@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 I will test tonight if I can fix it and post it inside this PR. |
so.. you're using webpack to build server code? |
/usr/bin/env node
still needed?
@davidmarkclements yup, to write ES6 node.js code and transpile it back to node.js v4 for AWS Lambda. |
can this help |
@davidmarkclements have tried it, but does not work. Have checked it twice. :/ |
@davidmarkclements the whole problem is that Something like #! /usr/bin/env node
var pretty = require('./pretty.js')
module.exports = pretty Gonna have a try and run the tests. |
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 ( 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) |
I think it's a ridiculous situation (transpiling needs to stop), but I'm fine with a |
@joeyvandijk - you closed? |
@davidmarkclements apparently I have mistaken The correct code is now in #201 as described above by you and me. |
@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! |
Thanks for checking Pino out @theLarkin! |
It raises a great question. Should we treat it just like any other JS and strip the env? |
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 |
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>
.
|
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. |
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.