jorendorff / js-loaders

Pseudoimplementation of the proposed ES6 module loaders.
54 stars 7 forks source link

Errors rather than internally converting argument: Loader.set()/get()/has() #98

Closed johnjbarton closed 10 years ago

johnjbarton commented 10 years ago

In 1.6.3.11 Loader.prototype.set ( name, module )

The following steps are taken: 
1. Let loader be this Loader. 
2. ReturnIfAbrupt(loader). 
3. Let name be ToString(name). 

IMO we should not be converting arguments. We should not allow non-string arguments; we should throw is the argument is not a string. Having modules stored / retrieved under keys like "[object Window]" or "[object Object]" will lead to difficult to debug errors. (I assume ToString() causes conversion, the doc does not say).

arv commented 10 years ago

That is just how JS behaves. We should follow suite for consistency.

johnjbarton commented 10 years ago

Sorry I don't follow your reasoning. What part of JS prevents us from throwing an error if the argument is not a string?

arv commented 10 years ago

Almost all functions in ES5 and the DOM that expects a string accepts any object and it will call toString on it as needed. On Jan 13, 2014 10:40 AM, "johnjbarton" notifications@github.com wrote:

Sorry I don't follow your reasoning. What part of JS prevents us from throwing an error if the argument is not a string?

— Reply to this email directly or view it on GitHubhttps://github.com/jorendorff/js-loaders/issues/98#issuecomment-32180166 .

jorendorff commented 10 years ago

I'm with jjb in spirit but we're not inventing a language from scratch here. There is no way we are going to break with how every other JS API in the spec (and the universe at large) behaves.

johnjbarton commented 10 years ago

But you'll return iterators when other APIs return arrays?

arv commented 10 years ago

Other APIs return iterators. Map, Set and Array all return iterators.

johnjbarton commented 10 years ago

Well other new ES6 APIs return iterators. Given that inconsistency, then it makes sense for Loader to follow.