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

Added option to load/save build diff stats #21

Merged
merged 15 commits into from
Aug 14, 2019

Conversation

kuldeepkeshwar
Copy link
Contributor

@kuldeepkeshwar kuldeepkeshwar commented Jun 14, 2019

With this developer can load the stats & save them back.

stats format:(As mentioned here)

{
  timestamp: 123456789,
  files: [
    {
     filename: 'foo/foo.a1e6f.chunk.js',
     previous: 1000,
     size: 1024,
     diff: 24
    },
    // .. etc ..
  ]
}

Example:

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

This should solve #15 & #6

@kuldeepkeshwar
Copy link
Contributor Author

Seems like we have out-dated snapshots.
@developit should I update snapshots?

@developit
Copy link
Collaborator

@kuldeepkeshwar yup, good to update!

@kuldeepkeshwar
Copy link
Contributor Author

kuldeepkeshwar commented Jun 14, 2019

By default, when Webpack runs with mode equals to production, it writes to disk(size-plugin.json).
example:

          new SizePlugin({
            filename: "stats-lock.json" // 🙃 optional, default is size-plugin.json
          })

@kuldeepkeshwar
Copy link
Contributor Author

This also resolves #22

@kuldeepkeshwar
Copy link
Contributor Author

@developit can you please review it

* mjs to js

* updated package.json

* fixed typo (,)

* added publish-size

* publish size for master branch

* fixed typo

* fixed payload for size api

* size-store api configurable via env variable

* avoid publishing in test environment

* added todo

* pushed filename to store

* checked file before writing/reading

* corrected file write condition

* updated test

* moved fs-extra to dependencies

* added filename to diff payload

* add publish flag

* add writeFile option

* js to mjs
@kuldeepkeshwar
Copy link
Contributor Author

@developit summarizing the final changes:

  • options.filename: filename to save filesizes to disk, default size-plugin.json
  • options.publish: option to publish filesizes to size-plugin-store, default true
  • options.writeFile: option to save filesizes to disk, default true
  • include deleted files in diff process.

Copy link

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@developit
Copy link
Collaborator

Only thing I think we should consider is defaulting options.publish to false. Most folks using this plugin (via other CLI's, etc) would not expect it to publish to a remote server by default.

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

Good point, it should definitely be opt-in instead of opt-out 👍

@kuldeepkeshwar
Copy link
Contributor Author

@developit, addressed the review comments & updated code accordingly

@kuldeepkeshwar
Copy link
Contributor Author

@developit should we also make options.writeFile opt-in ?

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

@kuldeepkeshwar Not sure on that one actually. One option here that might help simplify would be if we removed options.writeFile, and simply had options.filename trigger the writing of stats files. It could be undefined by default, which means when the user specifies filename:'xx' they get the stats file.

options.publish:true could also detect when options.filename is unset and assign it to some internal value like .size-plugin.json since it needs that file on disk.

kuldeepkeshwar and others added 2 commits August 6, 2019 19:05
Co-Authored-By: Jason Miller <developit@users.noreply.github.com>
@kuldeepkeshwar
Copy link
Contributor Author

@developit, can we merge this? let me know if you have other thoughts in mind.

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