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

Make the distribution file smaller #212

Closed
koresar opened this issue Jun 1, 2016 · 23 comments
Closed

Make the distribution file smaller #212

koresar opened this issue Jun 1, 2016 · 23 comments

Comments

@koresar
Copy link
Member

koresar commented Jun 1, 2016

stampit v2 full minified is 13K.
The stampit v3 full minified is 44K.

We are using _.mergeWith. But stampit v2 uses lodash v3, and stampit v3 uses lodash v4.
Looks like lodash v4 is a bulky thing unlike lodash v3.

I've taken a look at the lodash implementation of the _.mergeWith. It does a lot of extra checks which in our case is not really necessary. We should do the following:

  1. Copy-paste the reference implementation of stamp specification.
  2. Remove the stamp-specification dependency from stampit.
  3. Use a different _.mergeWith implementation (e.g. lodash v3, or lodash.mergewith module, or some other thing).
  4. (???) With the new rollup build we might even remove the lodash dependency altogether. Not sure about that one though.

P.S. Could be babel-polyfil fault too. :)

@danielkcz
Copy link
Contributor

danielkcz commented Jun 1, 2016

  • Dropping transform-runtime alone it went down to 30K minified. That's 14K for just polyfills.
  • Dropping mergeWith alone it went down to 20K minified. That's 24K just for lodash stuff!
  • Thus dropping both of these produces minifed bundle of 5K 😲

Considering that transform-runtime is harder to replace. Finding alternative for mergeWith is indeed step in a right direction. If we use something else for merge then there isn't a reason to remove lodash as it will be using only isObject and isFunction which are quite small.

Personally I wouldn't be dropping stamp-specification as dependency. It would make it only harder to maintain if something changes.

@JosephClay
Copy link
Contributor

Do we get significant savings if we change all usages of lodash to lodash/function so that we're not bundling the entire library?

e.g.

// not this
const _ = require('lodash');
// _.mergeWith

// this
const mergeWith = require('lodash/mergeWith');

@koresar
Copy link
Member Author

koresar commented Jun 2, 2016

@JosephClay this might sound weird, but

const _ = require('lodash');

generates smaller distributive files than the

const mergeWith = require('lodash/mergeWith');

AFAIK the magic is cast by this line:

"babel-plugin-lodash": "^3.1.4",

@ericelliott
Copy link
Contributor

Personally I wouldn't be dropping stamp-specification as dependency. It would make it only harder to maintain if something changes.

compose() is moving from stamp-specification to stamp-utils. I agree it's OK to keep it as a dependency -- but stamp-specification is going to stop serving the compose() function.

@danielkcz
Copy link
Contributor

@JosephClay Yea unfortunately lodash is not ready for real tree shaking yet, so it has to be hacked through that babel plugin.

compose() is moving from stamp-specification to stamp-utils. I agree it's OK to keep it as a dependency -- but stamp-specification is going to stop serving the compose() function.

Um so stamp-specification would be just written manual how to do it? I hope that stamp-utils don't become dependency of stampit, that sounds kinda weird to me.

@koresar
Copy link
Member Author

koresar commented Jun 2, 2016

Um so stamp-specification would be just written manual how to do it?

That would be ideal. But sounds complicated. :)

@danielkcz
Copy link
Contributor

I am confused now, what would be complicated? There is already written manual of specification. By removing functional code from there makes it "written manual of how to do it".

I meant more like that making stamp-utils seems like bloating stampit package bit more with stuff that some people might not ever use. If they want it, they can use that dependency itself. However that means stampit will need to implement spec on its own. Is that the plan?

@koresar
Copy link
Member Author

koresar commented Jun 2, 2016

There is already a "manual": https://medium.com/@koresar/fun-with-stamps-episode-4-implementing-stamps-in-30-loc-e52f5c17dcfe

However that means stampit will need to implement spec on its own. Is that the plan?

Yes.

@ericelliott
Copy link
Contributor

stamp-specification is just the spec, going forward. stamp-utils is a tiny collection of low level tools for working with stamps that other libs (like stampit) should feel free to depend on.

@ericelliott
Copy link
Contributor

stampit will need to implement spec on its own. Is that the plan?

I thought it was interesting to have different people working from the spec writing independent stamp implementations just to test the spec and make sure the core team understood it well enough to write compatible implementations, but I don't care whether or not we actually keep a bunch of independent implementations of the spec going forward.

We're all busy people. Let's do the simplest thing that works. =)

@koresar
Copy link
Member Author

koresar commented Jun 2, 2016

it was interesting to have different people working from the spec writing independent stamp implementations

Oh, yeah!

Btw, I already found problems in the check-compose module. Will talk about it later. :)

@danielkcz
Copy link
Contributor

Alright, fair enough. Anyway that got bit side tracked here from main issue of size, but that needs to be tackled mainly in stamp-specification / stamp-utils first.

@koresar
Copy link
Member Author

koresar commented Jun 12, 2016

Removed lodash, polyfills, unused helper methods, etc. Now the minified stampit is 5.7K

$ ls -l dist/
-rw-r--r--  1 vasyl  staff  12784 Jun 12 22:23 stampit.es5.js
-rw-r--r--  1 vasyl  staff  18529 Jun 12 22:23 stampit.es5.js.map
-rw-r--r--  1 vasyl  staff  13682 Jun 12 22:23 stampit.full.js
-rw-r--r--  1 vasyl  staff  21246 Jun 12 22:23 stampit.full.js.map
-rw-r--r--  1 vasyl  staff   5874 Jun 12 22:23 stampit.full.min.js
-rw-r--r--  1 vasyl  staff   9872 Jun 12 22:23 stampit.js
-rw-r--r--  1 vasyl  staff  18244 Jun 12 22:23 stampit.js.map
-rw-r--r--  1 vasyl  staff   9788 Jun 12 22:23 stampit.mjs
-rw-r--r--  1 vasyl  staff  18241 Jun 12 22:23 stampit.mjs.map

Lodash.

  • Reimplemented isFunction and isObject. As the result no lodash dependencied in the src/stampit.js file.
  • The src/compose.js were using lodash/assign and lodash/mergeWith.
    • The assign was easy to replace. I copy-pasted the Sindre Sohus implementation proposed by @FredyC.
    • The merge was copy-pasted form deepmerge module found by @FredyC. It is implemented almost as we need it. But not exactly. There are tests still failing. Working on them now.

I'm pretty happy with 44->5.7 KB file size improvement. :)
We can squeeze 1KB more I think.

Let me know what you think. https://github.com/stampit-org/stampit/tree/v3_0

I'd like to thank @FredyC for doing all that rollup initial work. Very helpful!

@koresar
Copy link
Member Author

koresar commented Jun 13, 2016

I'm wasting too much time on that babel, lodash, ES6, etc. This pushes me to the conclusion that using ES6 for stampit is a bad idea.
Take a look at the React or lodash for instance. Their source code is ES5.

I'm tempting to rewrite stampit v3 using ES5.

P.S.
Sorry for the quite a negative post. I've just calculated that I spend ~80% on fixing/improving the ES6 build system, and ~20% on the stampit roadmap.

@danielkcz
Copy link
Contributor

I am sorry you had to go through with that, I could have helped me as I have much more experience with build systems and how it works.

I see you got rid of stamp-specification as well. I am still not convinced about this step as it could make things easier for changes to spec. Anyway I think those utility function should perhaps go to stamp-utils?

@koresar
Copy link
Member Author

koresar commented Jun 13, 2016

Which exactly utility functions?

@danielkcz
Copy link
Contributor

Those you created instead of using lodash, but I guess that depends on if I can convince you of getting stamp-specification back into the mix. In case you want to keep it this way, it's not probably necessary.

@koresar
Copy link
Member Author

koresar commented Jun 13, 2016

  • The stamp-specification code was written in the most readable and easy to understand way. It is very important to keep it like that. So, I'm going to supply a PR which improves the readability but degrades the performance.
  • The stampit is our main module. It should be as performant as possible unlike the stamp-specification.
  • The stamp-utils is the small "utility belt" module for those people who don't want to bloat their (browser?) apps with all the powers of stampit, but wishes to write pretty code. So, stamp-utils would need the stamp-specification dependency to be replaced with something more optimized.

@danielkcz
Copy link
Contributor

Ok I see, that makes sense. 👍

@danielkcz
Copy link
Contributor

I am thinking if it's actually necessary to place code for all these functions to the root. I placed the isComposable and isStamp there because they might want to be used separately, but are you sure that other function will be used also? I would to say to move them to src and lower a clutter there.

@koresar
Copy link
Member Author

koresar commented Jun 13, 2016

@FredyC true. My bad. Will fix that. Thanks

@koresar
Copy link
Member Author

koresar commented Jun 13, 2016

Talked with @FredyC in the gitter chat. As the result build system:

  • Works on all node.js and npm versions.
  • The minified browser version of stampit is 6K. The gzipped is 3.1K.
  • Things to do:
    • Support for Symbols.
    • Move files from root to src directory.

@koresar
Copy link
Member Author

koresar commented Jun 13, 2016

Closing this as Done.

@koresar koresar closed this as completed Jun 13, 2016
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

No branches or pull requests

4 participants