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

Adding history for builds into json #15

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mick-feller
Copy link

This should resolve: #6

It's the first pass where i choose json, let me know any comments or adjustments you like to make.

} catch (e) {
throw e;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think its good to promisify all fs methods so we can make use of async/await function,
let me know your view 👍

Copy link
Author

Choose a reason for hiding this comment

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

Good point i made some changes, i didn't use promisify util to make sure it can still support older versions of node. I've also updated the readme file to show the option let me know what you think

@mick-feller
Copy link
Author

@developit I opted for JSON here initially, we can extend with dotfiles if desired if you can give me some pointers on how you want to have the files formatted.

@timarney
Copy link

@developit Odds of getting this merged? Was looking into doing this exact thing.

@timarney
Copy link

timarney commented Dec 10, 2018

Just took a look at this 👍

One issue:
If the dir you're storing the file in doesn't exist it will throw (likely intended ... but might be nice to create the dir)

Couldn't store history: Error: ENOENT: no such file or directory, open '/repo-name/reports/build-sizes.json'

Also IMO would be nice to save the the diff info for the file size like the console output shows against the previous entry. Picturing being able to use this to chart file size changes overtime.

{
filename: x 
filesize: x
diff: (+4.26 kB) 
}

Current console output

static/nnw6w_3RdMEf0sZwVrMJh/pages/index.js ⏤  1.23 kB (+1.23 kB)
static/nnw6w_3RdMEf0sZwVrMJh/pages/pdf.js ⏤  4.26 kB (+4.26 kB)
static/nnw6w_3RdMEf0sZwVrMJh/pages/_error.js ⏤  2.43 kB (+2.43 kB)

Current JSON

 "1544468967650": {
    "static/AmCaWNJU20AT31h6SVe31/pages/index.js": 1233,
    "static/AmCaWNJU20AT31h6SVe31/pages/pdf.js": 4263,
    "static/AmCaWNJU20AT31h6SVe31/pages/_error.js": 2428
  }

@mick-feller
Copy link
Author

mick-feller commented Dec 10, 2018

I can make those changes... you need to give me a bit of time though.

Question do you still want to keep the timestamp in the json? or do you just want to do an array of items and add the timestamp as a key in the object something like this:

Adding the diffInBytes and Pretty both to the object.

The missing dir i will fix. Let me know what you think

{
   filename: x 
   filesize: x
   diff: (+4.26 kB) 
   diffBytes: 1233
   timestamp: 123456789
}

@timarney
Copy link

timarney commented Dec 10, 2018

First - thanks for your work on this :)

Was trying it out and was happy to see there was already a PR to add writing to a file support.

In most cases I think an array of items to loop over would be perfect.

{
   filename: x 
   filesize: x
   diff: (+4.26 kB) 
   diffBytes: 1233
   timestamp: 123456789
}

@timarney
Copy link

Had another thought on this ...

What if this built up a array and passed the results to a callback

Allowing the user to save to a file or db or whatever they want

new SizePlugin({ cb: (jsonObj) =>{
//save to json or whatever you like
}
})

...
let obj = {
              filename: name,
              filesize: size,
              diff: deltaText,
              diffBytes: delta,
              color,
              timestamp: this.buildTimestamp
               };

              jsonOutput.push(obj);     
...

this.cb(jsonOutput);

You could create an addon for save to json with the readFile etc... keeping this repo all that code out of the main repo.

@developit
Copy link
Collaborator

developit commented Dec 11, 2018

I like the updated format suggestion @timarney. One nit though: it's super easy to format the output and I'd rather keep that separate from the JSON. Instead, we should have the JSON include the before & after sizes, both compressed and uncompressed:

{
  // we can hoist the timestamp, it's constant for all files
  timestamp: 123456789,
  files: [
    {
     filename: 'foo/foo.a1e6f.chunk.js',
     previous: 1000,
     size: 1024,
     diff: 24
    },
    // .. etc ..
  ]
}

Seem reasonable @mick-feller + @timarney?

Oh also I love the idea of allowing devs to hook the output saving. Easiest way to do that is to allow options to specify load() and save():

new SizePlugin({
  json: true,
  async load() {
    return JSON.parse(await someDb.get('stats'));
  },
  async save(stats) {
    await someDb.set('stats', JSON.stringify(stats));
  }
})

That way internally it's just:

import fs from 'fs-promise';
const filename = options.filename || 'size.json';
const loaded = options.load ? options.load() : this.readFile(filename);
let data = await loaded;
if (typeof data === 'string') data = JSON.parse(data);
// use data here

src/index.mjs Outdated Show resolved Hide resolved
@timarney
Copy link

Nice ya agreed on the format as you've outlined.

Before & after sizes makes the most sense.

Co-Authored-By: mick-feller <[email protected]>
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@mick-feller
Copy link
Author

So i did a first pass on this one, not totally done yet don't have more time tonight.

I remapped the json to the desired format, i've also added that if the previous file size comes back as zero or undefined, i'll try to read the json from the location and pick up the previous file size from the json. If that fails set it to current file size.

I still need to do the load and save outside, i'll work on that tomorrow or later this week, i just wanted to give you guys a peek already to see if we are getting close.

@timarney
Copy link

timarney commented Jan 2, 2019

We're doing some work with a forked version of this repo https://github.com/timarney/size-plugin/blob/master/src/index.mjs#L177 here https://github.com/cds-snc/bundle-size-tracker

Roughly we're writing a JSON file and than reading / writing the filesizes to a DB and than charting them + connecting them to Github.

In our case from this PR we need the JSON write to file but not the read. Which make me think as per #15 (comment) splitting out the save / read functionality is a good solution.

In our case we don't need much info in the JSON file

fileSizes.push({
			  filename: assetNames[i],
			 filesize: size
			});

Needed to pull the filename form assetNames to get the original name of the file.

@ianwalter
Copy link

Hi @mick-feller, what about using something like conf to handle the (default) storage details/logic?

@@ -68,6 +68,7 @@
"escape-string-regexp": "^1.0.5",
"glob": "^7.1.2",
"gzip-size": "^5.0.0",
"jsx": "^0.9.89",
"minimatch": "^3.0.4",

Choose a reason for hiding this comment

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

why is jsx installed?

@cb-eli
Copy link

cb-eli commented May 20, 2019

Can anyone merge this feature please?

@kuldeepkeshwar
Copy link
Contributor

@developit @mick-feller anyone working on this? I can pick it up as I need similar feature at work

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.

Store the output
9 participants