Skip to content

Conversation

@erikkemperman
Copy link
Member

I would like to propose some changes to resolve-options shortly, one of which is to avoid binding function options quite so often. In fact I think we should bind per resolver, not per option (and certainly not per option-invocation, as is currently the case!). This PR would facilitate that. Thoughts?

@phated
Copy link
Member

phated commented Jul 19, 2017

@erikkemperman why do we need this custom binding logic? Can't we just forward the this context and only use .apply throughout? That method is fast, as far as I'm aware.

@erikkemperman
Copy link
Member Author

@phated Yeah, I looked at that first -- but I did not see a way to do that without a breaking change, namely having a new compulsory argument to pass context to normalize.

But if that would be better than this bind hack, sure. The corresponding PR on resolve-options would become a little bit simpler as well.

@phated
Copy link
Member

phated commented Jul 19, 2017

@erikkemperman not sure what you mean? If we call valor.normalize.apply(context, arguments) and use apply internally, that would forward the context.

@phated
Copy link
Member

phated commented Jul 19, 2017

I'll put together a PR after lunch.

@erikkemperman
Copy link
Member Author

Ah, but that doesn't work for the built-in types (already bound to null) and we'd have a weird assymmetry between normalize and normalize.boolean et.al.; we can pass context to normalize via .call() and .aply() but the same would not work for normalize.[built-in].

@phated
Copy link
Member

phated commented Jul 19, 2017

@erikkemperman why can't the built-in types be applied the same way?

@erikkemperman
Copy link
Member Author

@phated They're pre-bound, before export, to null (in master) so invoking call or apply with a context will make no difference.

@erikkemperman
Copy link
Member Author

Or did I misunderstand the question?

@phated
Copy link
Member

phated commented Jul 19, 2017

@erikkemperman I've submitted a separate PR (#9) because I think code speaks louder than words 😛 Hope you have a chance to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants