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

Check if process is defined for better browser compatibility #60

Closed
wants to merge 1 commit into from

Conversation

RomanHotsiy
Copy link

Added a little extra check to prevent process is not defined error when the library is included in the browser build by webpack 5.

@jorgebucaran
Copy link
Owner

Thanks, @RomanHotsiy. Mind explaining why we need that to work?

@RomanHotsiy
Copy link
Author

Sure, @jorgebucaran.

I use this library in a package that can be used in node and browser. Most users of the lib use Webpack for browser bundles. Webpack 4 automatically included process polyfill which is not the case for Webpack 5 anymore.

So the code fails in runtime: 'process is not defined'. This can be fixed in Webpack config itself but it complicates the setup for every user.

This change adds a simple check if the process variable is defined. It doesn't change anything for node env but does not crash in browsers or other envs where the process may be not definied.

@jorgebucaran
Copy link
Owner

Thanks for the explanation, @RomanHotsiy.

@jorgebucaran jorgebucaran added the enhancement New feature or request label Jun 14, 2021
@kibertoad
Copy link
Collaborator

@jorgebucaran Do you think this change should be included? I can rebase it if you want.

@jorgebucaran
Copy link
Owner

I've been so used to using the global process object I had forgotten we could import it.

Wouldn't it be better to import { env, platform } from "process"?

@kibertoad
Copy link
Collaborator

@jorgebucaran does that work with CJS require as well? if yes, that would be simpler, yes.

@jorgebucaran
Copy link
Owner

Yes. We just need to change how index.cjs is currently compiled to CommonJS.

What's Pino's deprecation roadmap for CommonJS, by the way?

@kibertoad
Copy link
Collaborator

@jorgebucaran Yes, likely late 2022, after LTS lands.

@jorgebucaran
Copy link
Owner

Nice. Exciting times ahead! 💯

@jorgebucaran
Copy link
Owner

I might've fixed this with cf35254. Please have a look. Now available in 2.0.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants