-
Notifications
You must be signed in to change notification settings - Fork 22
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
Feature request to pass HTTP_ORIGIN with CORS #10
base: master
Are you sure you want to change the base?
Conversation
|
||
//push on to stack | ||
if (origin) { | ||
requestIds.push({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend using an object hash instead of an array here. That way we won't need to iterate/filter the array later, we can just pull out the request by its ID:
requestIds = {};
requestIds[ 'ID_'+info.requestId ] = { origin: origin, expiration:1234 };
//when retrieving value
var request = requestIds[ 'ID_' + info.requestId ];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with a hash - but then you'd have to use delete() to remove items. Since the list isn't every going to be huge the looping speed will be small. Using an array - you can use the Array methods like '.find' and '.filter' ...
I don't like to remove items in a loop like this:
for (var id in hash) {
delete(hash[id]);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to loop through the IDs, you'll have the specific ID you need to delete:
delete requestIds[ 'ID_' + info.requestId ];
//or if you don't care about having an empty key
requestIds[ 'ID_' + info.requestId ] = null;
I don't see any situation in the code where you would iterate all of the keys...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about when you have a request but no response? Your requestIds will grow ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would have to loop through the Object - looking for expired (non-response) - so since the list will be at most 100 (100 requests in 10s) - looping isn't a big deal ... using a hash seems like a good idea until you do for (var id in hash)
- then looping is better in an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid point, the request should be removed from the requestIds
inside the getRequestAndCleanUp
method then.
Did you intend to periodically clean out 'expired' requests? I don't see that logic here
This will find the element and filter expired ones and the current one ... This may be better (less compares):
|
Logic was ok ... filter is weird as it true (keeps) and false (removes) |
Chris,
I added the feature. The issue with the response it that you don't have the request object:
http://sysmagazine.com/posts/166539/
So I did the following: