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

Performance Port: Adds Performance.now, Performance.mark, & Date.now #5

Closed
wants to merge 4 commits into from
Closed

Performance Port: Adds Performance.now, Performance.mark, & Date.now #5

wants to merge 4 commits into from

Conversation

aterranova-bv
Copy link
Contributor

JIRA

No real associated ticket, but is a part of https://bits.bazaarvoice.com/jira/browse/SCP-1281

Details

This PR ports the performance.now, performance.mark, and date.now modules out of firebird and into bv-ui-core.

The modules use the native Navigation Timing API when available and provide polyfills when necessary. They provide a consistent interface to callers so that the underlying implementation details aren't of concern to calling code.

Verification

Run the tests: npm run test

All tests should pass.

* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/now
*/
// Cache references for speed and to guard against shenanigans
var nativeDate = global.Date; // TODO: where does global come from?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We haven't yet decided on how we want to handle global / window handling.

Needs a solution before accepting PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, let's open a separate PR that adds a module like https://github.com/bazaarvoice/scoutfile/blob/master/lib/browser/global.js. This is not optimal, but thankfully it's a one-liner.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did wonder how terrible it would be to make scoutfile a dependency, to directly pull in this module, knowing full well that code using the scoutfile as a peer dependency would also be using it. It sounds like npm v3 will flatten it out safely, but would that seem like a reasonable approach to getting at global (and also in the future, things like IE detection if needed etc)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how Webpack will end up consolidating those, but I'm open to it if there's a good story there.

@brianarn
Copy link
Contributor

I'd like to see us improve test coverage of this before accepting it. By running node_modules/.bin/tap --cov test/unit/performance you can get a clear picture of what coverage looks like so far.

performance-coverage

@aterranova-bv
Copy link
Contributor Author

Oooh, that's a cool tool. I'll add that to the main documentation if it's not already there.

Looks like the native implementations aren't being covered, which makes sense at this point because in order for us to use them, performance and performance.now need to be available.

Likely won't be able to get full coverage on these until we add browser tests.

@aterranova-bv
Copy link
Contributor Author

This PR also relies on the global module to be merged first - not sure how to indicate that.

@brianarn
Copy link
Contributor

Once the global module is available, we can likely use proxyquire or something like that to fake the native performance so that we can get 100% code coverage, even in node.

@rmurphey
Copy link
Contributor

Note that there's now a pull request open to get browser tests working, which involves a switch from tap to tape. This will need to be adapted in a very minor way to work with that.

Performance Port: removes use strict declarations

Performance Port: Fixes linting errors and require paths

Performance Port: restructures the directory structure and adds performance.mark module and tests

Performance Port - restructures directory, updates documentation
@aterranova-bv
Copy link
Contributor Author

PR rebased on and adjusted for new global module and karma + phantomjs.

The code coverage still isn't 100%.
I tried both:

  1. proxyquire - https://github.com/thlorenz/proxyquire
    • Didn't play nice with webpack. Forget the exact errors now.
  2. inject-loader - https://github.com/plasticine/inject-loader
    • We run into an issue where a dependency and a mocked version of the same dependency cannot be run together, even from separate test files.

I'm going to try rewire-webpack now.

Update: rewire-webpack worked in the browser but not in PhantomJS. Using the set function caused errors immediately. I ran into the same problem as this.

@rmurphey rmurphey closed this in 7e5da3d Jul 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants