Reduce bytes #401

carhartl · 2018-02-02T09:17:07Z

The goal is to reduce bytes without compromising on readability.

carhartl · 2018-02-02T09:20:22Z

carhartl · 2018-02-02T09:48:46Z

Ah, the latest commit doesn't seem to help with the gzipped sized... so I can just as well remove it again from the PR, though I like how key + value assignments stand on the same line now.

carhartl · 2018-02-02T09:52:54Z

No, if I interprete it correctly, compared to the latest minified release, 2.2.0, we'll get a decrease of 6 bytes when doing a new release with these changes? @FagnerMartinsBrack

FagnerMartinsBrack · 2018-02-02T11:04:07Z

... though I like how key + value assignments stand on the same line now

I have just one point that's probably a matter of style, also probably be subjective and I have no evidence to support it:

I've seen people having trouble to understand a codebase with lots of ternary statements. An explicit if is easier to reason about. I would say that, because it's subjective, we can sacrifice explicit ifs in favor of ternary, but when it doesn't make any difference is it worth keeping the explicit if?

Not strong about it, though. It achieved a good result in reducing 5 bytes :)

carhartl · 2018-02-02T11:17:37Z

We're using said operator in other places as well, those weren't replaced with explicit if statements either. Thus, imo, the code is even more consistent now. I heard the argument about ternary vs if before and won't deny it, but readability in this case much depends on how huge the operands get and/or whether you start nesting ternary operators. Consistency is equally important to me.

carhartl · 2018-02-02T11:19:20Z

I'm not merging yet, because I wanted to take another look, maybe I find another place to take away some bytes.

FagnerMartinsBrack · 2018-02-02T11:20:20Z

... but readability in this case much depends on how huge the operands get and/or whether you start nesting ternary operators

Agreed

carhartl · 2018-02-02T11:51:50Z

We could do

if (typeof attributes.expires === 'number') {
    attributes.expires = new Date(new Date().getTime() + attributes.expires * 864e+5);
}

and this would result in reduction of 17 bytes altogether in this branch. I'm not too happy about creating two Date instances, but does it really matter? I would have used Date.now() instead of new Date().getTime() but that is only supported in IE 11...

carhartl · 2018-02-02T11:54:50Z

new Date(new Date() * 1 + attributes.expires * 864e+5), ha

FagnerMartinsBrack · 2018-02-02T11:55:26Z

I don't think there's any problem with more Date instances. There are limits for cookies so one is not supposed to be calling Cookies.get() many times.

carhartl · 2018-02-02T11:55:45Z

I guess that would be ok because that line in particular was already obfuscated by that attributes.expires * 864e+5.... :)

carhartl · 2018-02-02T11:58:19Z

carhartl · 2018-02-02T12:01:23Z

I have to agree though that new Date(new Date() * 1 + attributes.expires * 864e+5) interferes with:

The goal is to reduce bytes without compromising on readability.

🙈

FagnerMartinsBrack · 2018-02-02T12:04:46Z

Honestly, as long as it has tests and doesn't break other use cases, I'm fine with it. This codebase is heavily trading off legibility for less bytes already so I don't think it makes much difference.

carhartl · 2018-02-02T20:46:45Z

Interesting, I was experimenting with making the extend() function smaller:

function extend () {
	var i = 0;
	var result = {};
	while (arguments[i]) {
		for (var key in arguments[i]) {
			result[key] = arguments[i][key];
		}
		i++;
	}
	return result;
}

With that I'm getting a test failure

>> remove - deletion
>> Message: should delete the cookie
>> Actual: "c="
>> Expected: ""

and I can't figure out why! That function works fine when experimenting in the (node) console, e.g. it produces the same result as the former one... what am I overlooking?

carhartl · 2018-02-02T20:49:49Z

Ah, it was because attributes was undefined in the test case.

carhartl · 2018-02-02T20:52:28Z

..and then the changes would turn into a -21 👎

This is fun. :)

carhartl · 2018-02-04T11:10:02Z

We could change collecting the result(s) to this, would increase savings to 27:

diff --git a/src/js.cookie.js b/src/js.cookie.js
index 4618358..5a90898 100644
--- a/src/js.cookie.js
+++ b/src/js.cookie.js
@@ -38,7 +38,6 @@
 
 	function init (converter) {
 		function api (key, value, attributes) {
-			var result;
 			if (typeof document === 'undefined') {
 				return;
 			}
@@ -58,7 +57,7 @@
 				attributes.expires = attributes.expires ? attributes.expires.toUTCString() : '';
 
 				try {
-					result = JSON.stringify(value);
+					var result = JSON.stringify(value);
 					if (/^[\{\[]/.test(result)) {
 						value = result;
 					}
@@ -90,16 +89,13 @@
 
 			// Read
 
-			if (!key) {
-				result = {};
-			}
-
 			// To prevent the for loop in the first place assign an empty array
 			// in case there are no cookies at all. Also prevents odd result when
 			// calling "get()"
 			var cookies = document.cookie ? document.cookie.split('; ') : [];
 			var rdecode = /(%[0-9A-Z]{2})+/g;
 			var i = 0;
+			var jar = {};
 
 			for (; i < cookies.length; i++) {
 				var parts = cookies[i].split('=');
@@ -121,18 +117,15 @@
 						} catch (e) {}
 					}
 
+					jar[name] = cookie;
+
 					if (key === name) {
-						result = cookie;
 						break;
 					}
-
-					if (!key) {
-						result[name] = cookie;
-					}
 				} catch (e) {}
 			}
 
-			return result;
+			return key ? jar[key] : jar;
 		}
 
 		api.set = api;

FagnerMartinsBrack · 2018-02-04T12:53:45Z

Sounds great! You're having a lot of fun ain't you? :D

carhartl · 2018-02-09T00:25:48Z

With that we'd be at -44:

diff --git a/src/js.cookie.js b/src/js.cookie.js
index 4618358..9d48419 100644
--- a/src/js.cookie.js
+++ b/src/js.cookie.js
@@ -38,7 +38,6 @@
 
 	function init (converter) {
 		function api (key, value, attributes) {
-			var result;
 			if (typeof document === 'undefined') {
 				return;
 			}
@@ -58,7 +57,7 @@
 				attributes.expires = attributes.expires ? attributes.expires.toUTCString() : '';
 
 				try {
-					result = JSON.stringify(value);
+					var result = JSON.stringify(value);
 					if (/^[\{\[]/.test(result)) {
 						value = result;
 					}
@@ -90,19 +89,15 @@
 
 			// Read
 
-			if (!key) {
-				result = {};
-			}
-
 			// To prevent the for loop in the first place assign an empty array
 			// in case there are no cookies at all. Also prevents odd result when
 			// calling "get()"
 			var cookies = document.cookie ? document.cookie.split('; ') : [];
 			var rdecode = /(%[0-9A-Z]{2})+/g;
-			var i = 0;
+			var jar = {};
 
-			for (; i < cookies.length; i++) {
-				var parts = cookies[i].split('=');
+			while (cookies.length) {
+				var parts = cookies.pop().split('=');
 				var cookie = parts.slice(1).join('=');
 
 				if (!this.json && cookie.charAt(0) === '"') {
@@ -111,8 +106,7 @@
 
 				try {
 					var name = parts[0].replace(rdecode, decodeURIComponent);
-					cookie = converter.read ?
-						converter.read(cookie, name) : converter(cookie, name) ||
+					cookie = (converter.read || converter)(cookie, name) ||
 						cookie.replace(rdecode, decodeURIComponent);
 
 					if (this.json) {
@@ -121,18 +115,11 @@
 						} catch (e) {}
 					}
 
-					if (key === name) {
-						result = cookie;
-						break;
-					}
-
-					if (!key) {
-						result[name] = cookie;
-					}
+					jar[name] = cookie;
 				} catch (e) {}
 			}
 
-			return result;
+			return key ? jar[key] : jar;
 		}
 
 		api.set = api;

This removes the break statement for when having found the desired cookie, and this would eventually decrease performance in theory, although only in cases of the cookie we're searching for being more towards the head of the list...

carhartl · 2018-02-09T00:40:26Z

Another thing I noticed... @FagnerMartinsBrack

We have

attributes = extend({
    path: '/'
}, api.defaults, attributes);

but why is path: '/' not part of defaults right away (de facto it is a default)?

carhartl · 2018-02-09T00:46:24Z

Ah, the test for API for changing defaults told me...

carhartl · 2018-02-09T16:06:38Z

-25

I'm reluctant to remove

if (key === name) {
    break;
}

from the loop, which would give us -35, but would have aforementioned performance implications.

carhartl · 2018-02-10T20:26:49Z

-30 was my goal, we have -33 now.

carhartl · 2018-02-11T08:36:49Z

feel free to do a git merge

Not sure I understand you. Are you asking to merge these changes manually instead of going ahead and merging them right here?

carhartl · 2018-02-11T09:06:49Z

Regarding Browserstack, it's not possible testing IE 6 + 7 without buying a plan:

My Parallels does in fact no longer work, the (old) version I have is not working in High Sierra. I don't have an idea how to test that. I do not even have any licensed Windows XP version any longer, otherwise I could be using Virtualbox I guess.

You cannot even get these versions from https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/

FagnerMartinsBrack · 2018-02-11T12:03:40Z

Not sure I understand you. Are you asking to merge these changes manually instead of going ahead and merging them right here?

No no. Feel free to merge from here. The commits from this PR are atomic so it should be fine to have them on master.

As a sidenote:

What I have been done so far is to close the PR from a commit on master with the message Closes gh-xx where xx is the target PR number (see here for an example). Things are much easier if you fetch all the PRs from upstream, then it's just a matter of doing git cherry-pick <hash> on master and push it.

The reason I've been doing this is to be able to standardize the commit messages on master. Github has changed several times how the merge/rebase message is created.

However, it should be fine to merge from the Github UI with or without Squash. Up to you.

carhartl · 2018-02-11T16:10:00Z

Regarding Browserstack, it's not possible testing IE 6 + 7 without buying a plan

I've signed up and contacted them, asking for an open source plan for this repository. It's possible to use it with Travis. In case we will get access, I would prepare everything to switch from Saucelabs. We would be able to automate testing browsers including IE 6 and 7 again.

carhartl · 2018-02-11T16:34:08Z

Tried converting my Paralles images to VirtualBox but all I got is a bluescreen.

carhartl · 2018-02-11T16:35:44Z

Running out of ideas, I believe I'm going to wait a little more and see whether I get to test on BrowserStack... in the meantime I'm dreaming of removing support for Internet Explorer 6 + 7.

FagnerMartinsBrack · 2018-02-12T02:44:15Z

I'm dreaming of removing support for Internet Explorer 6 + 7

We can do it, it will just require a major bump to 3.0.0, that's it.

What if you leave the [].slice change to milestone 3.0.0 and then commit everything else and release the version 2.2.1?

^{It's better to use the milestones to help to remember what to put in the Releases changelog :D}

carhartl · 2018-02-12T07:21:35Z

What if you leave the [].slice change to milestone 3.0.0 and then commit everything else and release the version 2.2.1?

I still have the ambition to find out about [].slice vs arguments anyway. And, taking a second look, it looks like it's possible to download and run Windows XP without needing a license in VirtualBox: https://www.makeuseof.com/tag/download-windows-xp-for-free-and-legally-straight-from-microsoft-si/

And then, the people from BrowserStack have answered already btw:

We are happy you reached out!

BrowserStack loves open source, and has sponsored thousands of projects. Here is more information about our offering and activation process.

What you get for free: Unlimited manual and automated testing for open source websites and mobile apps. Full platform access to Live, Automate, App Live and App Automate (coming soon). Total of 5 user licenses and parallels.

What we request in return: Give us a shoutout on your GitHub project page or website. Post and hyperlink our attached logo, and a line about how you use BrowserStack. We are counting on you to help us spread the word to the open source community.

Next steps:
Sign up for a BrowserStack free trial account.
Add a status badge to your project, if you're using Automate.
Send us your free trial account email ID and the updated GitHub page.

^ If you think it's worth pursuing further, let me know, @FagnerMartinsBrack!

carhartl · 2018-02-12T07:25:25Z

Oh, ok, it seems you can just download Windows 7 from https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/, and in Windows 7 itself there's a possibility to run Windows XP, maybe that's the reason why there were no XP images.

FagnerMartinsBrack · 2018-02-12T07:49:14Z

If you think it's worth pursuing further, let me know, @FagnerMartinsBrack!

I think it is if you're up to it :D.

Do they have a fix for this? Can they support IE6+?

carhartl · 2018-02-12T08:16:24Z

Do they have a fix for this?

I hope, can‘t tell yet. I also remember we had problems running the full test suite in Saucelabs for PRs from outside the original repo, is that still the case?

Can they support IE6+?

Yes, they do! (But not in the trial, only for paid plans and, as far as I understood open source plans, hence the idea to switch.)

carhartl · 2018-02-12T08:23:24Z

I also remember we had problems running the full test suite in Saucelabs for PRs from outside the original repo

Unless we choose to keep the access key in plain text, which we don’t, nope!

https://docs.travis-ci.com/user/browserstack/

carhartl · 2018-02-16T07:50:52Z

At the moment the test suite doesn't even run in IE6, because of Object.keys():

js-cookie/test/utils.js

Line 38 in f9f3f0e

Object.keys(Cookies.get()).forEach(Cookies.remove);

carhartl · 2018-02-16T07:56:49Z

But once that's fixed, I conclude this branch works fine in IE 6 with all the changes.

This way value + key appear together, visually.

Collecting all cookies in a jar, and when looking for a specific one breaking out of the loop. By this we're also sharing one variable less (`result`) for both the write and read functionality, which will make it easier to modularize the library in the future.

Falsey is sufficient.

carhartl · 2018-02-16T08:04:28Z

But once that's fixed

=> 6e42d24

carhartl · 2018-02-16T08:07:39Z

Browserstack is amazing. I'm working on moving our testing in CI to it next.

carhartl requested a review from FagnerMartinsBrack February 2, 2018 10:41

FagnerMartinsBrack approved these changes Feb 2, 2018

View reviewed changes

FagnerMartinsBrack mentioned this pull request Feb 2, 2018

Prevent XSS in the cookie attributes #400

Closed

carhartl added this to the v2.2.1 milestone Feb 4, 2018

carhartl force-pushed the reducing-bytes branch 2 times, most recently from 87c1060 to 7b23c3a Compare February 9, 2018 16:24

carhartl force-pushed the reducing-bytes branch from 8da5321 to ff0aec0 Compare February 11, 2018 08:39

carhartl force-pushed the reducing-bytes branch from ff0aec0 to 37bf5a4 Compare February 11, 2018 10:29

carhartl force-pushed the reducing-bytes branch from 37bf5a4 to d36f69f Compare February 15, 2018 07:33

carhartl added 7 commits February 16, 2018 09:03

Make use of chaining for string wrangling

d53efd3

Make use of ternary operator instead of if/else

7441ec7

This way value + key appear together, visually.

Make expires attribute handling more concise

4cd7df2

Refine code for applying read converters

7f75645

Remove superfluous assignment

1df2c68

Falsey is sufficient.

Pass on arguments without array'izing it

a7ab62c

carhartl force-pushed the reducing-bytes branch from d36f69f to a7ab62c Compare February 16, 2018 08:03

carhartl merged commit 3ec5018 into js-cookie:master Feb 16, 2018

carhartl deleted the reducing-bytes branch February 16, 2018 08:07

willstocks mentioned this pull request Mar 19, 2019

[Question] For loop vs Array.forEach #497

Closed

Reduce bytes #401

Reduce bytes #401

Conversation

carhartl commented Feb 2, 2018 • edited

carhartl commented Feb 2, 2018

carhartl commented Feb 2, 2018 • edited

carhartl commented Feb 2, 2018 • edited

FagnerMartinsBrack commented Feb 2, 2018

carhartl commented Feb 2, 2018

carhartl commented Feb 2, 2018

FagnerMartinsBrack commented Feb 2, 2018

carhartl commented Feb 2, 2018 • edited

carhartl commented Feb 2, 2018

FagnerMartinsBrack commented Feb 2, 2018

carhartl commented Feb 2, 2018

carhartl commented Feb 2, 2018

carhartl commented Feb 2, 2018 • edited

FagnerMartinsBrack commented Feb 2, 2018

carhartl commented Feb 2, 2018

carhartl commented Feb 2, 2018

carhartl commented Feb 2, 2018

carhartl commented Feb 4, 2018

FagnerMartinsBrack commented Feb 4, 2018

carhartl commented Feb 9, 2018

carhartl commented Feb 9, 2018

carhartl commented Feb 9, 2018 • edited

carhartl commented Feb 9, 2018

carhartl commented Feb 10, 2018

carhartl commented Feb 11, 2018 • edited

carhartl commented Feb 11, 2018 • edited

FagnerMartinsBrack commented Feb 11, 2018

carhartl commented Feb 11, 2018 • edited

carhartl commented Feb 11, 2018

carhartl commented Feb 11, 2018

FagnerMartinsBrack commented Feb 12, 2018 • edited

carhartl commented Feb 12, 2018

carhartl commented Feb 12, 2018 • edited

FagnerMartinsBrack commented Feb 12, 2018

carhartl commented Feb 12, 2018 • edited

carhartl commented Feb 12, 2018

carhartl commented Feb 16, 2018 • edited

carhartl commented Feb 16, 2018

carhartl commented Feb 16, 2018

carhartl commented Feb 16, 2018

carhartl commented Feb 2, 2018 •

edited

carhartl commented Feb 2, 2018 •

edited

carhartl commented Feb 2, 2018 •

edited

carhartl commented Feb 2, 2018 •

edited

carhartl commented Feb 2, 2018 •

edited

carhartl commented Feb 9, 2018 •

edited

carhartl commented Feb 11, 2018 •

edited

carhartl commented Feb 11, 2018 •

edited

carhartl commented Feb 11, 2018 •

edited

FagnerMartinsBrack commented Feb 12, 2018 •

edited

carhartl commented Feb 12, 2018 •

edited

carhartl commented Feb 12, 2018 •

edited

carhartl commented Feb 16, 2018 •

edited