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

move common logic to size-plugin-core & add support for size-plugin bot #4

Merged
merged 35 commits into from
Oct 5, 2019

Conversation

kuldeepkeshwar
Copy link
Contributor

@kuldeepkeshwar kuldeepkeshwar commented Aug 14, 2019

I have created a bot, which comments the sizes of the assets and the changes since the last build into the relevant PR

Support for the bot is already added to the webpack's size-plugin here

Summarizing the changes:

luwes
luwes previously approved these changes Aug 25, 2019
Copy link
Owner

@luwes luwes left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM

@luwes luwes dismissed their stale review August 25, 2019 16:13

render chunk missing?

@luwes
Copy link
Owner

luwes commented Aug 25, 2019

got an error npm linking this version.

Screen Shot 2019-08-25 at 12 18 01 PM

@kuldeepkeshwar
Copy link
Contributor Author

@luwes it seems to work on my end. can you share the repo where you are trying?

@luwes
Copy link
Owner

luwes commented Aug 25, 2019

@kuldeepkeshwar it's because outputOptions.dir is undefined on my end. In a lot of cases this will be the case I think, not everyone sets that in the rollup config.

Maybe better go back to path.dirname(outputOptions.file)?

This is the repo: https://github.com/luwes/sinuous-todomvc

@luwes
Copy link
Owner

luwes commented Aug 25, 2019

or maybe output.dir || path.dirname(outputOptions.file)?

@kuldeepkeshwar
Copy link
Contributor Author

@luwes handled both cases(dir/file)

@luwes
Copy link
Owner

luwes commented Aug 25, 2019

hmm, it's reading all the files in the dir. should only be the ones generated from rollup.

Screen Shot 2019-08-25 at 1 20 48 PM

@kuldeepkeshwar
Copy link
Contributor Author

hmm, it's reading all the files in the dir. should only be the ones generated from rollup.

Screen Shot 2019-08-25 at 1 20 48 PM

@luwes this is bit tricky, I will come back on it

@kuldeepkeshwar
Copy link
Contributor Author

@luwes you can use the exclude option to ignore files

bundleSize({exclude:'!bundle.js'})

Note: delta not only includes the files which are created/updated in the current build but it also includes the files which are deleted in the current build & to do so plugin needs to consider all files in the directory to figure out which files are deleted in the current build.

@kuldeepkeshwar kuldeepkeshwar changed the title add support for size-plugin bot move common logic to size-plugin-core & add support for size-plugin bot Sep 2, 2019
@luwes
Copy link
Owner

luwes commented Sep 8, 2019

very cool, will test this soon

@luwes
Copy link
Owner

luwes commented Sep 14, 2019

@kuldeepkeshwar this looks good, only the output has to be only the current build.
they are build files in /dist so they can't be excluded but I only like to see the currently building file in the output.

what's the use case of wanting to see a fully deleted build file?

Screen Shot 2019-09-14 at 10 53 16 AM

@kuldeepkeshwar
Copy link
Contributor Author

@luwes it should only show the delta(addition/modification/deletion).
If all those files are deleted in current build then they should come in delta.

Can you please share a repo link, I would like to dig it deeper

@kuldeepkeshwar
Copy link
Contributor Author

kuldeepkeshwar commented Oct 1, 2019

@luwes help me close this.

@luwes
Copy link
Owner

luwes commented Oct 1, 2019

@kuldeepkeshwar sorry for the late response. can I be of help to get the output issue fixed? it has to be included before we can merge this PR. it's the luwes/sinuous repo.

@kuldeepkeshwar
Copy link
Contributor Author

kuldeepkeshwar commented Oct 4, 2019

@luwes I have fixed it. Needed to handle the case of the single output file separately.

Ref:
Screen Shot 2019-10-05 at 1 42 26 AM

@luwes
Copy link
Owner

luwes commented Oct 5, 2019

Awesome! almost there, could you just add these changes?

diff --git a/rollup-plugin-size.js b/rollup-plugin-size.js
index 839adc4..9afbf06 100644
--- a/rollup-plugin-size.js
+++ b/rollup-plugin-size.js
@@ -20,11 +20,11 @@ const defaults = {
  * @param {boolean} [options.publish] option to publish filesizes to size-plugin-store
  * @param {boolean} [options.writeFile] option to save filesizes to disk
  */
-function bundleSize(_options) {
-  const coreOptions = Object.assign(defaults, _options);
+function bundleSize(options) {
+  const coreOptions = Object.assign(defaults, options);
   coreOptions.compression = coreOptions.brotli ? 'brotli' : 'gzip';
 
-  const core = new SizePluginCore(_options);
+  const core = new SizePluginCore(coreOptions);
   async function generateBundle(outputOptions, bundle) {
     try {
       const assets = Object.keys(bundle).reduce((agg, key) => {

@luwes
Copy link
Owner

luwes commented Oct 5, 2019

also on my end the newlines are too far apart. for a single output file it can be without an extra newline.

Screen Shot 2019-10-04 at 8 44 23 PM

@luwes
Copy link
Owner

luwes commented Oct 5, 2019

could I also be added as maintainer to https://github.com/kuldeepkeshwar/size-plugin-core?

@kuldeepkeshwar
Copy link
Contributor Author

@luwes updated the variable names.

Regarding the spacing, size-plugin uses this format & rollup-plugin-size has a different format (without extra newline character). we need to stick on one format. size-plugin-core was created by abstracting out functions from size-plugin.

could I also be added as a maintainer to https://github.com/kuldeepkeshwar/size-plugin-core?

yes, just create a pull request. I will add you.

@luwes
Copy link
Owner

luwes commented Oct 5, 2019

Awesome thanks! Great work 💪

@luwes luwes merged commit 40905d2 into luwes:master Oct 5, 2019
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.

Share core functionality with size-plugin?
2 participants