-
Notifications
You must be signed in to change notification settings - Fork 147
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
Add an optional group to the status key to allow separation of prefetch status from normal page status. #117
Add an optional group to the status key to allow separation of prefetch status from normal page status. #117
Conversation
*/ | ||
spf.net.resource.prefix = function(type, label) { | ||
return type + '-' + label; | ||
spf.net.resource.key = function(labels) { |
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.
WDYT of implementing this with var_args? The "arguments" object is array-like, but not sure if spf.array handles it well enough.
I like not having to wrap this method's input in an array each and every time.
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.
why not simply
return type + '-' + label + (opt_group ? '-' + opt_group : '');
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.
+1 that makes more sense, so that the order of the input is somewhat enforceable. Also O(1) instead of O(n) 😉
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.
@nicksay I felt that the function had an awkward name/use case combination. The name implied some generic prefixing capability, but really only built keys from one specific combination. In fact, many of the users refer to the output as 'key'.
So instead I figured it would make more sense as a generic key maker for resources. Which would work equally well for any set of labels that were being sent to it. I wouldn't necessarily be to opposed to
spf.net.resource.key = function(type, label, opt_group)
But, I would put money on a new variable being needed soon enough, and we'd revisit it again.
@rviscomi Added in var args.
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.
Perhaps, but given the broad usage, I'd prefer to avoid the repeated array creation and iteration. Also, the tighter type-annotation of the arguments version will allow better optimization and error checking. (We should probably actually change the second arg to be {string} to allow possible string joining during compilation.)
No problems with renaming the function.
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.
Alright fair enough. I went to back to specified parameters, and left it as spf.net.resource.key.
e5e3f77
to
3ed244e
Compare
LGTR |
… status from normal page status. Closes youtube#116.
3ed244e
to
c88ce50
Compare
LGTM |
Add an optional group to the status key to allow separation of prefetch status from normal page status.
Closes #116