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

Pino browser fixes #650

Merged
merged 2 commits into from
May 14, 2019
Merged

Pino browser fixes #650

merged 2 commits into from
May 14, 2019

Conversation

davidmarkclements
Copy link
Member

fixes #647 and #649

NOTE: MERGE and RELEASE hapijs/hapi-pino#79 FIRST

In #612 we added symbols to browser pino.

This is breaking backward compatibility, and rather than add additional bytes for a polyfill to the payload overhead that was already added by including all symbols I think we should remove the symbols from the browser as they were only included for a "very niche use case" in #612.

We can instead update hapi-pino to use Symbol.for since the serializers symbol is a global symbol.

This also includes the smallest possible polyfill for globalThis for a standard-compliant cross platform global, see #649 (comment)

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jfahrenkrug
Copy link

@davidmarkclements Not that I'm a reviewer or anything, but what do you think about adding a comment to polyfillGlobalThis() that links to https://mathiasbynens.be/notes/globalthis? I think if anyone comes across that code down the line they'd be grateful to have a thorough explanation of the zaniness that is going on ;-)

@@ -162,7 +162,6 @@ pino.levels = {
}

pino.stdSerializers = stdSerializers
pino.symbols = require('./lib/symbols')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a fear that this makes this change semver-major.
I would prefer it not to be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. But I think this is a case of semver purity vs practicality. polyfilling symbols is extra overhead and not fully possible (e.g. typeof symbol). There is only one use case, and it's technically server side - so I think an argument can be made in this case for considering the breaking of browser compat a bug, and removing this to be the solution to that bug. Since this change along with the hapi-pino change will literally affect no one, as there are no other cases for pino.symbols in the browser.

@davidmarkclements davidmarkclements merged commit 0b62bd8 into master May 14, 2019
@mcollina mcollina deleted the pino-browser-fixes branch May 14, 2019 11:20
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide transpiled browser version
4 participants