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

Save some bytes by declaring variable as extra function argument #519

Merged
merged 3 commits into from
Dec 31, 2017

Conversation

okwolf
Copy link
Contributor

@okwolf okwolf commented Dec 30, 2017

Also modified a test to check for a case that another proposed byte reduction change didn't handle.

-3 bytes

they don't grow on trees you know
@okwolf okwolf requested a review from jorgebucaran December 30, 2017 07:27
@codecov
Copy link

codecov bot commented Dec 30, 2017

Codecov Report

Merging #519 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #519   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         135    135           
  Branches       40     40           
=====================================
  Hits          135    135
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7edf32...2935888. Read the comment docs.

src/index.js Outdated
@@ -12,8 +12,7 @@ export function h(name, props) {
for (i = node.length; i--; ) {
stack.push(node[i])
}
} else if (null == node || true === node || false === node) {
Copy link
Owner

Choose a reason for hiding this comment

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

@okwolf Nice try, but this won't work when the text node is 0.

Copy link
Contributor Author

@okwolf okwolf Dec 30, 2017

Choose a reason for hiding this comment

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

@jorgebucaran we're missing a test that should fail with this PR then.

@okwolf
Copy link
Contributor Author

okwolf commented Dec 30, 2017

@jorgebucaran I updated the test for h covering numbers so that all the use cases for the logic I tried to simplify are now covered. I wasn't able to cut the size so I reverted that change, but I left the other for a smaller 3 byte diet.

@jorgebucaran
Copy link
Owner

Thanks! I feel we are doing that trick of passing fake arguments to functions too much. I'm going to merge this, but I may revert it later, along with the others to increase readability if it only means some bytes.

@jorgebucaran
Copy link
Owner

@okwolf Can you change the commit message (and PR title) to something less mischievous? 🙏

@okwolf okwolf changed the title Two can play at this code golf game 🏌️‍♂️ Save some bytes by declaring variable as extra function argument Dec 30, 2017
@okwolf
Copy link
Contributor Author

okwolf commented Dec 30, 2017

@jorgebucaran oh, I was actually mostly joking with this PR to begin with. I changed the title, but I'd rather just make a new one for different commit messages/branch name.

I'm not super attached to the only actual byte shrinking change left, but I do want to get the gap in test coverage for h patched.

Copy link
Contributor

@infinnie infinnie left a comment

Choose a reason for hiding this comment

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

What about writing a helper function for "function" != typeof x or referencing the document in a local variable?

@jorgebucaran
Copy link
Owner

@infinnie I tried both approaches and neither yielded fewer bytes actually haha.

@okwolf
Copy link
Contributor Author

okwolf commented Dec 31, 2017

@jorgebucaran I tried both approaches and neither yielded fewer bytes actually haha.

Gzip can be funny sometimes about when it likes duplication and when it doesn't.

@jorgebucaran
Copy link
Owner

Merging. Thanks @okwolf! 🎉

@jorgebucaran jorgebucaran merged commit 79f5ea8 into jorgebucaran:master Dec 31, 2017
@okwolf okwolf deleted the code-golf branch December 31, 2017 01:05
@leeoniya
Copy link

leeoniya commented Dec 31, 2017

you guys are doing it all wrong. check out the gzthermal tool [1] used here [2].

::runs away and hides:: 🤣

[1] https://encode.ru/threads/1889-gzthermal-pseudo-thermal-view-of-Gzip-Deflate-compression-efficiency
[2] https://blog.usejournal.com/of-svg-minification-and-gzip-21cd26a5d007

@jorgebucaran
Copy link
Owner

@leeoniya If you are going to come tell people they are wrong but don't have the time to tell them exactly where or how then please don't. Unless you are Kaepora Gaebora.

@leeoniya
Copy link

i guess sarcasm doesnt come across well over the interwebs. i will improve my attempts at humor in the future.

anyways, i figured you guys might be interested in gzthermal since you're so OCD about byte size. but it can also lead to some very time-consuming code-golfing.

@infinnie
Copy link
Contributor

Or an IIFE…

@jorgebucaran
Copy link
Owner

jorgebucaran commented Dec 31, 2017

@leeoniya I like your humor, but you always seem to be in a rush when posting around here, aren't you? I think you are extremely smart, but when sharing wisdom, there's always so much more than what you reveal, which is inconvenient for those less enlightened (me). 😉

@leeoniya
Copy link

leeoniya commented Dec 31, 2017

i will also point out, that if you guys compress with closure compiler, you don't need to do these optimizations by hand. here's the pretty-printed hyperapp file compiled via Closure Compiler [1], it already does this:

  function y(a, d, c) {
    if ("string" === typeof a) {
      c = document.createTextNode(a);
    } else {
      c = (d = d || "svg" === a.name) ? document.createElementNS("http://www.w3.org/2000/svg", a.name) : document.createElement(a.name);
      a.props.oncreate && x.push(function() {
        a.props.oncreate(c);
      });
      for (var b = 0; b < a.children.length; b++) {
        c.appendChild(y(a.children[b], d));
      }
      for (var h in a.props) {
        E(c, h, a.props[h]);
      }
    }
    return c;
  }

[1] https://closure-compiler.appspot.com/code/jsca1d643ec6d5e87dac7ed5b01a23f25c/default.js

@jorgebucaran
Copy link
Owner

@infinnie That would be cool, but actually when you bundle Hyperapp with your app, it is fewer bytes because you are not using the UMD bundle. 😆

@jorgebucaran
Copy link
Owner

@leeoniya I've heard about the wonders of google closure compiler before, but last time I checked, it seemed too hard to setup. Also last time I looked at domvm I tried to see what you were doing, and you had a build script, which I want to avoid.

Having said that, it would be much nicer if we didn't have to write obfuscated code, so I'll have another look. 🤔

@infinnie
Copy link
Contributor

Maybe you could try a gzip directly after comment and extra space removal?

@leeoniya
Copy link

leeoniya commented Dec 31, 2017

domvm's build script does a lot more stuff. (plus it used to use the Java version of Closure Compiler, which is much faster)

rollup has a ready-made plugin for the the js version of Closure: https://www.npmjs.com/package/rollup-plugin-closure-compiler-js

@jorgebucaran
Copy link
Owner

@leeoniya I heard that the JS version of the compiler is not as good as the Java one. Do we get the same benefits you said above using the JS version?

@leeoniya
Copy link

in my testing, the js version only differed by encoding html entities within strings [1]. it was easy to work around in my build script via a simple regex replace (but i also didn't need to produce source maps for the minified builds since i only generate source maps for the unminified bundles).

[1] google/closure-compiler-js#79

@jorgebucaran
Copy link
Owner

@leeoniya How did you produce the code you posted here? I tried https://closure-compiler.appspot.com, but it gave all sort of warnings.

@leeoniya
Copy link

you can do hyperapp's npm run bundle and paste the non-minified output into there. (leave the commented header)

jorgebucaran pushed a commit that referenced this pull request Jan 7, 2018
)

* Save a few previous bytes they don't grow on trees you know.
* Update test to cover children with the number 0.
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.

4 participants