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

remove old callbacks in Request.JSONP.request_map #1315

Merged
merged 2 commits into from Aug 12, 2015

Conversation

@SergioCrisostomo
Copy link
Member

@SergioCrisostomo SergioCrisostomo commented Aug 11, 2015

fixes #1293

Add option to remove old Request.JSONP.request_map callbacks. Defaults to false to be BC.

@DimitarChristoff
Copy link
Member

@DimitarChristoff DimitarChristoff commented Aug 11, 2015

since that's not official API (the stored responses) BC should be ignored
and data should be removed by default. anyone who's augmented it to use
request_map to cache results clearly know what they are doing and can
adjust their code accordingly imo.

On Tuesday, August 11, 2015, Sergio Crisostomo notifications@github.com
wrote:

fixes #1293 #1293

Add option to remove old Request.JSONP.request_map callbacks. Defaults to

false to be BC.

You can view, comment on, or merge this pull request online at:

#1315
Commit Summary

  • remove old callbacks in Request.JSONP.request_map

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#1315.

Dimitar Christoff

"JavaScript is to JAVA what hamster is to ham"
@D_mitar - https://github.com/DimitarChristoff

@arian
Copy link
Member

@arian arian commented Aug 11, 2015

Agreed with @DimitarChristoff

@SergioCrisostomo
Copy link
Member Author

@SergioCrisostomo SergioCrisostomo commented Aug 11, 2015

Nice feedback! will update soonish and ping back.

@SergioCrisostomo SergioCrisostomo force-pushed the SergioCrisostomo:fix1293 branch 2 times, most recently from 61d5759 to b600ac5 Aug 11, 2015
@SergioCrisostomo
Copy link
Member Author

@SergioCrisostomo SergioCrisostomo commented Aug 11, 2015

@@ -119,19 +119,20 @@ Request.JSONP = new Class({
return !!this.running;
},

clear: function(){
clear: function(index){

This comment has been minimized.

@anutron

anutron Aug 11, 2015
Member

Hmmm.

What if the index isn't passed in? If it's required, that's a breaking change.

What if the index passed in isn't the running request?

This comment has been minimized.

@DimitarChristoff

DimitarChristoff Aug 11, 2015
Member

it's not breaking because worst case scenario, it will just try to delete an undefined property.

I have a better fix which does not change the API - storing the index within the script tag, then auto deleting

http://jsfiddle.net/dimitar/frxnqj5q/1/

This comment has been minimized.

@anutron

anutron Aug 11, 2015
Member

Good stuff. Might suggest we used data-jsonp-index or something rather than the element attribute, but whatever, it doesn't matter really.

This comment has been minimized.

@DimitarChristoff

DimitarChristoff Aug 11, 2015
Member

i considered it but it's so short lived - initially even thought of storage but that meant storage gc when element is removed. i'd agree about data-prop as it ought to be slightly cheaper.

This comment has been minimized.

@DimitarChristoff

DimitarChristoff Aug 12, 2015
Member

easier fix still. one liner

    Request.JSONP.request_map['request_' + index] = function(){
        delete Request.JSONP.request_map['request_' + index];
        this.success(arguments, index);
    }.bind(this);

so
+ delete Request.JSONP.request_map['request_' + index];

@@ -52,4 +57,29 @@ describe('Request.JSONP', function(){

});

it('should clear the request callback map', function(){

This comment has been minimized.

@DimitarChristoff

DimitarChristoff Aug 11, 2015
Member

why don't we use function(done){ ... done() ... } for async but we have waitsFor(<ambiguous number>, fn);

This comment has been minimized.

@SergioCrisostomo

SergioCrisostomo Aug 12, 2015
Author Member

@DimitarChristoff: we are still using Jasmine 1.3 and the done() API is from 2.0. We can do that upgrade in another PR, that would be good.

@SergioCrisostomo SergioCrisostomo force-pushed the SergioCrisostomo:fix1293 branch from b600ac5 to 37d2406 Aug 12, 2015
@SergioCrisostomo
Copy link
Member Author

@SergioCrisostomo SergioCrisostomo commented Aug 12, 2015

Updated again. No index anymore and using Dimitar's oneliner, much better idea.

@DimitarChristoff
Copy link
Member

@DimitarChristoff DimitarChristoff commented Aug 12, 2015

@SergioCrisostomo
Copy link
Member Author

@SergioCrisostomo SergioCrisostomo commented Aug 12, 2015

Nice! Yeah, that index was already there from before and is unused. Will cherry pick your work and use it. thanks!

Sent from my iPhone

On 12 aug 2015, at 12:19, Dimitar Christoff notifications@github.com wrote:

@SergioCrisostomo check master...DimitarChristoff:SergioCrisostomo-fix1293 - missed an index and such.


Reply to this email directly or view it on GitHub.

SergioCrisostomo added a commit that referenced this pull request Aug 12, 2015
remove old callbacks in Request.JSONP.request_map
@SergioCrisostomo SergioCrisostomo merged commit 5f286e7 into mootools:master Aug 12, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mootools mootools deleted a comment Jun 1, 2018
@mootools mootools deleted a comment Feb 13, 2019
@mootools mootools deleted a comment Jul 15, 2019
@mootools mootools deleted a comment Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.