Consider those who doesn't use semicolons by icamys · Pull Request #171 · js-cookie/js-cookie

icamys · 2016-04-08T15:57:18Z

Avoiding "intermediate value" error when used with other modules.
See http://stackoverflow.com/questions/23370269/jquery-autosize-plugin-error-intermediate-value-is-not-a-function

FagnerMartinsBrack · 2016-04-09T04:30:45Z

Thank you @icamys , can you please add a test that reproduces the problem when js-cookie is included after another JavaScript file that doesn't use semicolons?

icamys · 2016-04-09T18:48:34Z

Here's an example:
js-cookie-test.html.zip

In my project I use gulp and asset builder. As you probably know, when we execute gulp command, firstly all 'main' files from bower_components directory are concatenated, after that all client code is appended to it.

I found this problem, when during the concatination js-cookie main file was appended after JavaScript Canvas to Blob main file.

FagnerMartinsBrack · 2016-04-10T02:17:08Z

@icamys Sorry, but I meant a test here. We need a test that runs when the build runs on CI to prevent regressions. Even though it is just a semicolon, we need a testable example for posterity.

The Contributing Guidelines shows how to contribute to js-cookie, and there it is said that we need a test when proposing a feature through a Pull Request.

The best way to do it would be to simulate the concatenation or inclusion of the js-cookie file after another one that doesn't end with a semicolon and ends with an expression. Something like this would be enough:

var expression = function() {}
(function (factory) { ...

Can you do that and amend on this PR? Otherwise you can create an issue about this instead, a PR is meant to be just the implementation of the feature.

icamys · 2016-04-12T13:31:35Z

@FagnerMartinsBrack Today I've added a test, but I'm getting a TypeError from PhantomJS:

Testing http://127.0.0.1:9998/ .......>> PhantomJS error:
>>  TypeError: Object is not a function (evaluating '(function(){ return {}; })()') 
>>  [ { file: 'http://127.0.0.1:9998/missing_semicolon.html',
>>     line: 1,
>>     function: 'global code' },
>>   { file: '', line: 0, function: 'appendChild' } ]
F...................................
>> read - missing leading semicolon
>> Message: encounters `(intermediate value)...` error
>> Actual: undefined
>> Expected: true
>> http://127.0.0.1:9998/tests.js:75:21

I'm virgin with PhantomJS, that's why I don't really know how to fix this error, but it's definitely a real situation, that reproduces the problem.

The test is pretty simple. In HTML using ajax request I'm getting the contents of js-cookie.js file, then prepending to it an expression that reproduces the said error:

# test/missing_semicolon.html

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title></title>
    <script>
        var xmlHttp = new XMLHttpRequest();
        xmlHttp.open( "GET", '../src/js.cookie.js', false );
        xmlHttp.send( null );
        window.scriptContent = '(function (){ return {}; })() ';
        window.scriptContent += xmlHttp.responseText;
        window.scriptContent += ';window.ok = true;';

        var script = document.createElement('script');
        script.innerHTML = window.scriptContent;

        document.getElementsByTagName('head')[0].appendChild(script);
    </script>
</head>
<body>
</body>
</html>

If you can give me the cue how to bypass this error, I would be grateful.

By the way, here is the QUnit test itself:

# somewhere in test/tests.js...

// github.com/js-cookie/js-cookie/pull/171
QUnit.test('missing leading semicolon', function (assert) {
    assert.expect(1);
    var done = assert.async();
    // Sandbox in an iframe so that we can poke around with document.cookie.
    var iframe = document.createElement('iframe');
    iframe.src = 'missing_semicolon.html';

    addEvent(iframe, 'load', function () {
        assert.strictEqual(iframe.contentWindow.ok, true, 'encounters `(intermediate value)...` error');
        done();
    });
    document.body.appendChild(iframe);
});

FagnerMartinsBrack · 2016-04-12T13:52:40Z

If you can give me the cue how to bypass this error, I would be grateful.

@icamys There is no need to, you can use that test in this Pull Request, maybe just changing the message to explain what is going on. js-cookie is trying to execute the Object as a function because of the lack of semicolon, if you add the semicolon in the beginning of the file, it will pass.

Can you git amend this Pull Request with those tests so that we can review properly using Github diff?

Thank you!

icamys · 2016-04-12T17:52:54Z

@FagnerMartinsBrack I've amended this PR.

js-cookie is trying to execute the Object as a function because of the lack of semicolon, if you add the semicolon in the beginning of the file, it will pass.

Agree, though we can't be sure about the code, that is executed before the module from time to time. That's why there is a need for an extra semicolon.

FagnerMartinsBrack · 2016-04-13T11:01:57Z

though we can't be sure about the code, that is executed before the module from time to time

I don't understand what you mean, could you please elaborate? Thanks.

FagnerMartinsBrack · 2016-04-13T11:03:41Z

test/missing_semicolon.html

+        xmlHttp.send( null );
+        window.scriptContent = '(function (){ return {}; })() ';
+        window.scriptContent += xmlHttp.responseText;
+        window.scriptContent += ';window.ok = true;';


Is there really a reason for changing window.ok = true after it is loaded? Can't we just check if the test throws an Error?

Oh yeah you can't, since it is inside an iframe xD, correct me if I am wrong.

In this case I guess something like window.jsCookieLoadedSuccessfully will explain what this ok means when reading the test. Not a big deal though.

I found a good resource in how to get errors from iframe in case you are interested: http://stackoverflow.com/a/14041297

FagnerMartinsBrack · 2016-04-13T11:29:06Z

@icamys Thank you very much for the quality of this Pull Request :O. I have made some comments, looking forward on your feedback.

icamys · 2016-04-14T07:06:03Z

@FagnerMartinsBrack I've updated PR again considering your comments.

FagnerMartinsBrack · 2016-04-14T07:11:12Z

test/tests.js

+		iframe.contentWindow.onerror = function () {
+			loadedSuccessfully = false;
+		};
+		assert.strictEqual(loadedSuccessfully, true, 'can\'t throw Object is not a function error');


We usually don't escape single quotes, we use double quotes instead (yeah, pretty crazy, but the code was like this since forever xD)

icamys · 2016-04-15T20:31:07Z

@FagnerMartinsBrack I still hope that PR would be accepted =)

Added a test that triggers an error if before module there is no extra semicolon

FagnerMartinsBrack · 2016-04-16T04:29:09Z

Ugh! It seems I made a mistake with the double quotes! see here, we are not using double quotes anymore it seems =(.

Can you please revert that, to fix the code style error on CI? Sorry for that =/
Yeah, this PR will be accepted for v2.1.1, no worries :)

FagnerMartinsBrack · 2016-04-16T09:12:45Z

Nevermind, I can fix this. Thank you for the tests!

FagnerMartinsBrack added this to the v2.1.1 milestone Apr 9, 2016

FagnerMartinsBrack mentioned this pull request Apr 9, 2016

The library should not throw an error if the methods are used in node #164

Closed

FagnerMartinsBrack reviewed Apr 13, 2016
View reviewed changes

FagnerMartinsBrack changed the title ~~Update js.cookie.js~~ Consider those who doesn't use semicolons Apr 13, 2016

FagnerMartinsBrack reviewed Apr 14, 2016
View reviewed changes

Update js.cookie.js

7ebe0fd

Added a test that triggers an error if before module there is no extra semicolon

FagnerMartinsBrack closed this in dbce759 Apr 16, 2016

Conversation

icamys commented Apr 8, 2016

Uh oh!

FagnerMartinsBrack commented Apr 9, 2016

Uh oh!

icamys commented Apr 9, 2016

Uh oh!

FagnerMartinsBrack commented Apr 10, 2016

Uh oh!

icamys commented Apr 12, 2016

Uh oh!

FagnerMartinsBrack commented Apr 12, 2016

Uh oh!

icamys commented Apr 12, 2016

Uh oh!

FagnerMartinsBrack commented Apr 13, 2016

Uh oh!

FagnerMartinsBrack Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

FagnerMartinsBrack Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

FagnerMartinsBrack Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

FagnerMartinsBrack Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

FagnerMartinsBrack commented Apr 13, 2016

Uh oh!

icamys commented Apr 14, 2016

Uh oh!

FagnerMartinsBrack Apr 14, 2016

Choose a reason for hiding this comment

Uh oh!

icamys commented Apr 15, 2016

Uh oh!

FagnerMartinsBrack commented Apr 16, 2016

Uh oh!

FagnerMartinsBrack commented Apr 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

FagnerMartinsBrack commented Apr 16, 2016 •

edited

Loading