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

BrowserProcessor's 'performance' entry is always {} #55

Merged
merged 3 commits into from
May 24, 2018
Merged

Conversation

yched
Copy link
Contributor

@yched yched commented May 22, 2018

window.performance.memory is an object of class MemoryInfo, which doesn't serialize to JSON : JSON.stringify(window.performance.memory) is "{}"

We need to explicitly include the three properties.

@fgm
Copy link
Owner

fgm commented May 23, 2018

I had noticed that indeed.

  • Any idea why it doesn't serialize properly by default ?
  • And isn't there some more compact way to write this fix ?
  • Isn't there some per-browser variation on these subkeys ?
  • Small code formatting issue on bitHound linter

@fgm fgm added the bug label May 23, 2018
@yched
Copy link
Contributor Author

yched commented May 23, 2018

Any idea why it doesn't serialize properly by default ?

Not really, but the Chrome team seems to have intentianally designed window.performance.memory like a special snowflake (quantized values that only change every 20 minutes and hide small changes...)

And isn't there some more compact way to write this fix ?

No simple one, for the reasons above (super specific class)

The only generic way I found to access "all the properties in window.performance.memory" was Object.keys(Object.getPrototypeOf(window.performance.memory)), and then you still have to check whether they're a value or a function...

I do think the intended use is to access each known property individally and explicitly.
If a new property gets added later on, it will just have to be explicitly extracted here.

Isn't there some per-browser variation on these subkeys ?

Nope, window.performance.memory is Chrome-only at the moment. If later on some other browser happened to add window.performance.memory as well, but without, say, the jsHeapSizeLimit property, then window.performance.memory.jsHeapSizeLimit would just be undefined, no big deal.

Small code formatting issue on bitHound linter

Will fix it

@yched
Copy link
Contributor Author

yched commented May 23, 2018

About "some more compact way" :

  • lodash's pick(window.performance.memory, ['jsHeapSizeLimit', 'totalJSHeapSize', 'usedJSHeapSize']) would work, but I don't feel this really justifies adding the dependency. Your call :-)

  • ES6 deconstruction would work too :

if (window.performance && window.performance.memory) {
  const { jsHeapSizeLimit, totalJSHeapSize, usedJSHeapSize } = window.performance.memory;
  result.browser.performance =  { jsHeapSizeLimit, totalJSHeapSize, usedJSHeapSize };
}
else {
  result.browser.performance = {};
}

but since that requires a full-fledged if / then / else instead of the ternary, that's not really shorter :-)

@fgm
Copy link
Owner

fgm commented May 24, 2018

I think we want to put this under windows.performance.memory, not under window.performance, though ? Otherwise we're losing any other content present under that standard key https://developer.mozilla.org/en-US/docs/Web/API/Performance

Which also means we need to check if window.performance is present at all in the result: this might be missing in a virtual browser, e.g. during SSR.

@fgm
Copy link
Owner

fgm commented May 24, 2018

Discussed offline: since this overwriting is an earlier feature/bug, and we don't want to copy the whole window.performance object at this point, the change will just rename its stored result from performance to performance.memory, to leave room for other processors to insert the other parts of window.performance as desired.

fgm and others added 3 commits May 24, 2018 17:28
window.performance.memory is an object of class MemoryInfo, which doesn't serialize to JSON : `JSON.stringify(window.performance.memory)` is `"{}"`

We need to explicitly include the three properties.
@fgm fgm merged commit 897ee79 into fgm:master May 24, 2018
fgm added a commit that referenced this pull request May 24, 2018
@fgm
Copy link
Owner

fgm commented May 24, 2018

Merged.

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

Successfully merging this pull request may close these issues.

2 participants