Skip to content
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

feat: Support bundlers. #1209

Merged
merged 3 commits into from Nov 10, 2021
Merged

feat: Support bundlers. #1209

merged 3 commits into from Nov 10, 2021

Conversation

@ShogunPanda
Copy link
Contributor

@ShogunPanda ShogunPanda commented Nov 5, 2021

This PR add supports for using pino in bundled source files.
In particular:

  • It uses real-require to perform dynamic requires/import.
  • It add support for a global variable __bundlerPathsOverrides which bundler plugins can set to override require paths if needed.

Note that in order for this to fully work, pinojs/thread-stream#48 must be merged and released.

Fixes #1164
Fixes #1104

Copy link
Member

@mcollina mcollina left a comment

This needs automated tests

@jsumners
Copy link
Member

@jsumners jsumners commented Nov 5, 2021

Why are we patching around other tool's problems?

@ShogunPanda
Copy link
Contributor Author

@ShogunPanda ShogunPanda commented Nov 5, 2021

We're not patching around their problems.
We're being friendly and provide them a mechanism to integrate us without bad monkey-patching after embracing WorkerThreads caused serious issues.

@ShogunPanda
Copy link
Contributor Author

@ShogunPanda ShogunPanda commented Nov 5, 2021

Tests added.

Copy link
Member

@mcollina mcollina left a comment

Could you add docs?

@ShogunPanda
Copy link
Contributor Author

@ShogunPanda ShogunPanda commented Nov 5, 2021

Sure, will add them soon.

@jsumners
Copy link
Member

@jsumners jsumners commented Nov 5, 2021

We're not patching around their problems. We're being friendly and provide them a mechanism to integrate us without bad monkey-patching after embracing WorkerThreads caused serious issues.

I don't understand how checking for magic variables (that look to be intended as private) in the global object so that we can then use a patched up require function isn't patching around their issues. If that other tool works correctly, then it will work with all regular uses of require. None of our usage of require in Pino is abnormal.

@mcollina
Copy link
Member

@mcollina mcollina commented Nov 5, 2021

There is a missing webpack plugin that will make this possible.

Currently pino and transport do not work at all with bundled codebases.

@ShogunPanda
Copy link
Contributor Author

@ShogunPanda ShogunPanda commented Nov 5, 2021

@jsumners Yeah, Matteo is right. Sorry, I should have mentioned that as we speak I'm working on one of the plugin that will add this support to webpack.

@jsumners
Copy link
Member

@jsumners jsumners commented Nov 5, 2021

Currently pino and transport do not work at all with bundled codebases.

My question is: how is that the fault of Pino?

@mcollina
Copy link
Member

@mcollina mcollina commented Nov 5, 2021

My question is: how is that the fault of Pino?

It's not the fault of pino. This is not a bugfix but an additional feature. People will ask for this and there are valid usecases for bundling their codebase.

@jsumners
Copy link
Member

@jsumners jsumners commented Nov 5, 2021

@jsumners Yeah, Matteo is right. Sorry, I should have mentioned that as we speak I'm working on one of the plugin that will add this support to webpack.

This is the slippery slope I'm trying to guard against. This PR addresses webpack. What about parcel, esbuild, babel, and whatever else someone has cooked up? If they are not solved by this PR, then we end up having to add all sorts of hacks for every bundler that comes along.

@ShogunPanda
Copy link
Contributor Author

@ShogunPanda ShogunPanda commented Nov 5, 2021

Nope, the fix is universal. I already experimented for webpack and esbuild.
We're gonna provide the webpack (as is one of the most used, if not the first one) plugin, so that people will have a reference.

@mcollina
Copy link
Member

@mcollina mcollina commented Nov 5, 2021

The gist of the fix:

  1. make sure the bundler generate separate entry points for the workers
  2. make sure the pino knows where those files live

This is generic.

@ShogunPanda
Copy link
Contributor Author

@ShogunPanda ShogunPanda commented Nov 5, 2021

@jsumners You can take a look at pinojs/pino-webpack-plugin#1 to see how a bundler plugin might work.

@jsumners
Copy link
Member

@jsumners jsumners commented Nov 5, 2021

@jsumners You can take a look at pinojs/pino-webpack-plugin#1 to see how a bundler plugin might work.

Truthfully, no. I know nothing of webpack nor it's API.

I'm not going to block this work. I just disagree with it for the reasoning stated above.

@ShogunPanda
Copy link
Contributor Author

@ShogunPanda ShogunPanda commented Nov 5, 2021

Docs added as well.

README.md Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

lgtm

@mcollina mcollina merged commit 4e08b37 into pinojs:master Nov 10, 2021
12 checks passed
@simoneb
Copy link
Contributor

@simoneb simoneb commented Nov 11, 2021

@jsumners I'll chime in to provide some context around the underlying issue and why we thought it would be a good idea to address it.

First of all, this problem is introduced by pino@7. Why? Because using service workers requires physical files on the file systems. Hence, even though I can agree that bundling a Node application is not necessarily a great idea this change has broken all such usages of pino. And this is "pino's fault" if you wish.

Obviously, all such usages could just as well decide to not upgrade to pino@7, but this breaking change is not really something that was even foreseen, it was just realized afterwards. Because bundling an application is something that in some cases is actually a valid scenario, we decided to try to see what supporting it would imply.

What it implies is that the original requirement of worker threads to have the file run by the worker exist physically on the file system remains, hence we needed to find a way to make it happen when using a bundler, which turned out to be tricky but doable. It required the change contained in this PR for pino which can be used for all bundlers (or at least we hope so), and the rest will be done on the bundler's side.

For webpack, which is the only implementation we have so far, we created a plugin. https://github.com/pinojs/pino-webpack-plugin

For the other bundlers, which we are not actively working on, there will be similar approaches that we can follow. Hopefully with the webpack plugin we have paved the way for the other bundlers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants