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

Bump catapult/traceviewer to latest #723

Merged
merged 7 commits into from
Oct 1, 2016
Merged

Bump catapult/traceviewer to latest #723

merged 7 commits into from
Oct 1, 2016

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Sep 30, 2016

This'll allow us to migrate to the shared FMP and TTI calculations

edit: Babel transpiling added to the build script, in order to address rampant use of destructuring. :)

commit 4959b8b832af4f0d91cf1b2bc9bed10fbcccad2e
Author: sullivan <[email protected]>
Date:   Fri Sep 30 11:08:20 2016 -0700
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

(github really needs a per-file PR viewer)

needs LICENSE but otherwise LGTM.

@@ -1,27 +0,0 @@
Copyright 2015 The Chromium Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

I think this file needs to be kept

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

actually, looks like theres some node v4 and v5 issues with lack of strict mode and class/let/const declarations. Maybe we can insert 'use strict'; lines in the converter scripts?

@paulirish
Copy link
Member Author

use strict injected. license restored.

@paulirish
Copy link
Member Author

Failures in node 4 and 5:

/home/travis/build/GoogleChrome/lighthouse/lighthouse-core/third_party/traceviewer-js/value/histogram.js:493
      for (var [stat, option] of this.summaryOptions) {
               ^
SyntaxError: Unexpected token [

ugh. :/

@brendankenny
Copy link
Member

yeah, hmm, even with --harmony that won't work in v4 or v5. I don't see much choice except patching random spots until we're all in on node 6 or upstreaming changes where they break older node.

@samccone
Copy link
Contributor

samccone commented Sep 30, 2016

I don't see much choice except patching random spots until we're all in on node 6 or upstreaming changes where they break older node.

we could also compile our code and deliver a bundle before npm publish and test runs.

I can open a CL for this if you find this acceptable

@brendankenny
Copy link
Member

we're already running a conversion script for traceviewer. I'd rather just keep whatever we have to do in there

@samccone
Copy link
Contributor

we're already running a conversion script for traceviewer. I'd rather just keep whatever we have to do in there

https://github.com/GoogleChrome/lighthouse/blob/0295887b88eebf6d80146dd5b79ed6f018451d4b/lighthouse-core/scripts/build-traceviewer-module.js

Is more about converting a browser running system to work in node, and less about "compiling" javascript though.

@paulirish
Copy link
Member Author

All good here. @brendankenny PTAL

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

great detective work. LGTM 💯

@brendankenny brendankenny merged commit 67680a0 into master Oct 1, 2016
@brendankenny brendankenny deleted the bumptraceviewr branch October 1, 2016 00:59
@paulirish
Copy link
Member Author

paulirish commented Dec 2, 2016

For future reference, this PR updated catapult to 4959b8b832af4f0d91cf1b2bc9bed10fbcccad2e

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.

3 participants