fix: save artifacts and assets in output path #1601

patrickhulce · 2017-02-02T17:57:58Z

More changes needed I came across during WPT integration investigation.

Default report when output is pretty will use same name and location as output path.
Artifacts will use same name and location as output path or will use getFilenamePrefix and be suffixed with .artifacts.log
Assets will use same name and location as output path.

Examples

--output-path=~/mydir/foo.out --save-assets generates

~/mydir/foo.out
~/mydir/foo.report.html
~/mydir/foo-0.trace.json
~/mydir/foo-0.screenshots.html

--output-path=./report.json --output json --save-artifacts generates

./report.json
./report.artifacts.log

--save-artifacts generates

./<HOST>_<DATE>.report.html
./<HOST>_<DATE>.artifacts.log

patrickhulce · 2017-02-02T18:00:39Z

lighthouse-core/lib/asset-saver.js

      const traceData = data.traceData;
-      fs.writeFileSync(`${filenamePrefix}-${index}.trace.json`, JSON.stringify(traceData, null, 2));
-      log.log('trace file saved to disk', filenamePrefix);
+      const traceFilename = `${options.path}-${index}.trace.json`;


I'm not super thrilled about calling this mixture of path and prefix path but passing both options everywhere seemed like overkill. My arm could be twisted to revert though... :)

isn't it just passed to this one function? agreed that calling it path is kind of confusing

it's this and saveArtifacts and used back in bin.ts

brendankenny

initial comments. My brain may just be tired, but it does seem like some new variable names may add some clarity here...but it may also just be the nature of the save*() beast we've created.

brendankenny · 2017-02-03T02:27:42Z

lighthouse-cli/bin.ts

@@ -260,25 +260,31 @@ function runLighthouse(url: string,
    .then(chrome => chromeLauncher = chrome)
    .then(() => lighthouse(url, flags, config))
    .then((results: Results) => {
+      let promise = Promise.resolve(results);


maybe move all this printing stuff to its own function? Definitely getting hard to follow :)

brendankenny · 2017-02-03T02:28:10Z

lighthouse-cli/bin.ts

      // delete artifacts from result so reports won't include artifacts.
      const artifacts = results.artifacts;
      results.artifacts = undefined;

      if (flags.saveArtifacts) {
-        assetSaver.saveArtifacts(artifacts);
+        promise = promise.then(_ => assetSaver.saveArtifacts(artifacts, resolvedPath));


saveArtifacts is sync, so does wrapping it matter?

I suppose not, it just seemed like the odd duckling before

brendankenny · 2017-02-03T02:31:16Z

lighthouse-core/lib/asset-saver.js

 * @return {!Promise}
 */
-function saveAssets(artifacts, results) {
-  return prepareAssets(artifacts, results).then(assets => {
+function saveAssets(artifacts, options = {}) {


this doesn't appear to be treated as an optional parameter, so any need for the default?

yeah ended up not being optional anymore, I'll remove

brendankenny · 2017-02-03T02:33:04Z

lighthouse-core/lib/asset-saver.js

      const traceData = data.traceData;
-      fs.writeFileSync(`${filenamePrefix}-${index}.trace.json`, JSON.stringify(traceData, null, 2));
-      log.log('trace file saved to disk', filenamePrefix);
+      const traceFilename = `${options.path}-${index}.trace.json`;


isn't it just passed to this one function? agreed that calling it path is kind of confusing

brendankenny · 2017-02-03T02:34:59Z

lighthouse-cli/bin.ts

      }
      if (flags.saveAssets) {
-        return assetSaver.saveAssets(artifacts, results).then(() => results);
+        promise = promise.then(_ => assetSaver.saveAssets(artifacts, {
+          title: path.basename(resolvedPath),


not really clear what title is here, or why it's called that. file prefix?

it becomes the title of the screenshots HTML page, which was essentially this basename before but it needed all the results passed in to compute it

patrickhulce · 2017-02-03T18:32:21Z

alright @brendankenny moved some things around and just got rid of title completely for now, hopefully it's clearer now

brendankenny

some more minor stuff. Definitely like the flow of this better. pwmetrics-events.js threw a wrench in things, though :)

brendankenny · 2017-02-03T23:15:05Z

lighthouse-cli/bin.ts

+        resolvedPath.slice(cwd.length + 1) : resolvedPath;
+
+    // delete artifacts from result so reports won't include artifacts.
+    const artifacts = results.artifacts;


maybe keep this in the caller and pass in artifacts as a separate argument to saveResults? Feels weird to remove them from the results in a function that's just saving things

I think it feels weird have this part of the saving logic outside saveResults haha, compromise? results = Object.assign({}, results, {artifacts: undefined});

brendankenny · 2017-02-03T23:23:44Z

lighthouse-core/lib/asset-saver.js

@@ -90,24 +89,23 @@ img {
 /**
 * Save entire artifacts object to a single stringified file


maybe add a line to the jsdoc that artifacts will be saved at pathWithBasename + '.artifacts.log'

brendankenny · 2017-02-03T23:26:24Z

lighthouse-core/lib/asset-saver.js

@@ -131,19 +129,20 @@ function prepareAssets(artifacts, results) {
 /**
 * Writes trace(s) and associated screenshot(s) to disk.
 * @param {!Artifacts} artifacts
- * @param {!Results} results
+ * @param {string} pathWithBasename


same thing with jsdoc, pathWithBasename + '.trace.json' and screenshots (though I guess the index thing makes that slightly harder to understand :)

brendankenny · 2017-02-03T23:32:54Z

lighthouse-cli/bin.ts

+                     flags: {output: any, outputPath: string, saveArtifacts: boolean, saveAssets: boolean}) {
+    let promise = Promise.resolve(results);
+    const cwd = process.cwd();
+    const configuredPath = !flags.outputPath || flags.outputPath === 'stdout' ?


it would be great to get some of the PR's first comment captured here or in a doc on the method (readme too?).

brendankenny · 2017-02-03T23:57:07Z

lighthouse-cli/bin.ts

+        flags.outputPath.replace(/\.\w{2,4}$/, '');
+    const resolvedPath = path.resolve(cwd, configuredPath);
+    const pathWithBasename = resolvedPath.includes(cwd) ?
+        resolvedPath.slice(cwd.length + 1) : resolvedPath;


maybe i'm missing something obvious, but what does slicing accomplish here?

my read is that resolvedPath always includes cwd because we just did path.resolve on it.
and so we'll always slice,
taking the absolute file path back to a relative one.

This is 1) the same as path.relative(cwd, resolvedPath) AFAICT. but 2) i don't know why we nuke it, since its a more absolute location.

Just nuked because with the default options the logging seemed a bit verbose to restate your cwd, don't have particularly strong feelings about it though if we're fine with the absolute path being printed.

paulirish · 2017-02-04T01:52:23Z

lighthouse-cli/bin.ts

+    const cwd = process.cwd();
+    const configuredPath = !flags.outputPath || flags.outputPath === 'stdout' ?
+        assetSaver.getFilenamePrefix(results) :
+        flags.outputPath.replace(/\.\w{2,4}$/, '');


instead of this edit we could do...

const parsed = path.parse(flags.outputPath); parsed.base = parsed.base.replace(parsed.ext, ''); path.format(parsed);

but i don't know if that's really worth it. :)

I like the explicitness of it but not a huge fan of the triple lines, haha :)

paulirish · 2017-02-04T01:54:54Z

lighthouse-cli/bin.ts

+    results.artifacts = undefined;
+
+    if (flags.saveArtifacts) {
+      assetSaver.saveArtifacts(artifacts, pathWithBasename);


as mentioned offline, we can remove --save-artifacts flag and the functions if we want. AFAIK, they havent been used for ages and are only for debugging.

paulirish · 2017-02-04T02:01:44Z

lighthouse-cli/bin.ts

+        flags.outputPath.replace(/\.\w{2,4}$/, '');
+    const resolvedPath = path.resolve(cwd, configuredPath);
+    const pathWithBasename = resolvedPath.includes(cwd) ?
+        resolvedPath.slice(cwd.length + 1) : resolvedPath;


my read is that resolvedPath always includes cwd because we just did path.resolve on it.
and so we'll always slice,
taking the absolute file path back to a relative one.

This is 1) the same as path.relative(cwd, resolvedPath) AFAICT. but 2) i don't know why we nuke it, since its a more absolute location.

brendankenny · 2017-02-06T23:22:21Z

readme.md

@@ -164,6 +164,23 @@ Options:
                     installations are found                                               [boolean]
 ```

+### Output Path Examples
+
+`--output-path=~/mydir/foo.out --save-assets` generates


I don't believe this actually works, though, because we don't do ~ expansion

depends on your shell I suppose, should I change to an absolute path?

depends on your shell I suppose

ah, yeah, I have to do --output-path ~/mydir/foo.out for it to expand

brendankenny · 2017-02-06T23:28:35Z

lighthouse-cli/bin.ts

+    const audits = results.audits;
+    const artifacts = results.artifacts;
+    // remove artifacts from result so reports won't include artifacts.
+    results = Object.assign({}, results, {artifacts: undefined});


this is fine (it makes some sense, we are dropping artifacts since we don't want the them in the saved/printed results), but since this resolves on the changed results I don't see the difference functionally?

So it still seems weird, but now it's that perfX relies on that side effect of saveResults to not get 100MB reports :) Not sure of the optimal configuration here, though

only changes in the sense that the original results object is preserved, if we never intend artifacts on being consumed along with the rest of the results I'd advocate splitting them into distinct sets in the lighthouse return object. Since that'd probably break others at this point, I'll restructure a bit more instead.

brendankenny · 2017-02-06T23:39:34Z

lighthouse-cli/bin.ts

+        flags.outputPath.replace(/\.\w{2,4}$/, '');
+    const resolvedPath = path.resolve(cwd, configuredPath);
+    const pathWithBasename = resolvedPath.includes(cwd) ?
+        path.relative(cwd, resolvedPath) : resolvedPath;


I'm personally +1 for dropping this line since it definitely adds mental overhead when reading. If it's annoying in the log we could fix it where it's logged instead

brendankenny · 2017-02-06T23:40:52Z

lighthouse-cli/bin.ts

+      promise = promise.then(_ => Printer.write(results, 'html', `${pathWithBasename}.report.html`));
+    }
+
+    return promise.then(_ => Printer.write(results, flags.output, flags.outputPath));


you're inheriting this, but it seems kind of magical that we rely on Printer.write to return results. How do you feel about doing a resolve on results here?

removed the need for it to return anything at all

brendankenny

LGTM!

fix: save artifacts in output path

92fb9ab

patrickhulce changed the title ~~fix: save artifacts in output path~~ fix: save artifacts and assets in output path Feb 2, 2017

patrickhulce commented Feb 2, 2017

View reviewed changes

brendankenny requested changes Feb 3, 2017

View reviewed changes

patrickhulce added 2 commits February 3, 2017 10:30

feedback restructuring

415df64

remove stray title

6c707b8

patrickhulce added 3 commits February 3, 2017 10:33

rename outputPath

c3e18ff

Merge remote-tracking branch 'origin/master' into asset_path

407e409

fix merge conflicts

e311a48

brendankenny requested changes Feb 4, 2017

View reviewed changes

paulirish reviewed Feb 4, 2017

View reviewed changes

feedback

a6b2d34

brendankenny requested changes Feb 6, 2017

View reviewed changes

moar feedback

69ebf22

brendankenny approved these changes Feb 7, 2017

View reviewed changes

brendankenny merged commit 76fb69e into master Feb 7, 2017

brendankenny deleted the asset_path branch February 7, 2017 01:28

brendankenny mentioned this pull request Feb 7, 2017

Consolidate viewer functionality into main report #1594

Merged

		@@ -90,24 +89,23 @@ img {
		/**
		* Save entire artifacts object to a single stringified file

fix: save artifacts and assets in output path #1601

fix: save artifacts and assets in output path #1601

Conversation

patrickhulce commented Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Feb 3, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce commented Feb 2, 2017 •

edited

Loading