WIP: -p/--profile option starts v8/chromium/chrome compatible profiling.#485
WIP: -p/--profile option starts v8/chromium/chrome compatible profiling.#485StyXman wants to merge 4 commits intomapbox:masterfrom
Conversation
|
This should help fixing things like #483. |
nebulon42
left a comment
nebulon42
left a comment
There was a problem hiding this comment.
Have not tested it yet. Just a few remarks.
| .options('p', {alias:'profile', boolean:true, default:false, describe:'Generate v8 profile data that can be loaded in chromium/chrome\'s profiler for inspection.'}) | ||
| .options('q', {alias:'quiet', boolean:true, default:false, describe:'Do not output any warnings'}) | ||
| .options('ppi', {describe:'Pixels per inch used to convert m, mm, cm, in, pt, pc to pixels', default:90.714}); | ||
| .options('ppi', {describe:'Pixels per inch used to convert m, mm, cm, in, pt, pc to pixels', default:90.714}) |
There was a problem hiding this comment.
if you reorder options you should probably move that before q.
There was a problem hiding this comment.
heeh, I missed that. in any case the reordering could be removed of the PR if you want.
bin/carto
Outdated
| } | ||
|
|
||
| if (options.profile) { | ||
| const fs = require('fs'); |
There was a problem hiding this comment.
const is part of ES6, we are still using ES5 so it should be var.
| "readable-stream": "^2.3.3", | ||
| "sax": "~1.2.1", | ||
| "util-deprecate": "^1.0.2", | ||
| "v8-profiler": "^5.7.0" |
There was a problem hiding this comment.
Wouldn't this dependency suffice? Why do we need all the others?
There was a problem hiding this comment.
I had a problem while installing v8-profiler, it kept complaining about a missing dependency, so I just kept adding things to npm install until it worked. Also, notice that Travis and AppVeyor complain about v8-profiler, so probably we could leav it completely out, add a check in the code, and suggest the command to run.
3 participants
For some reason this breaks; I can't remember how. I will keep working on it later, I hope to add more info tonight.