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

require() should not accept configuration as a first argument #48

Closed
kitsonk opened this issue Feb 26, 2016 · 4 comments
Closed

require() should not accept configuration as a first argument #48

kitsonk opened this issue Feb 26, 2016 · 4 comments

Comments

@kitsonk
Copy link
Member

kitsonk commented Feb 26, 2016

According to the AMD Spec, require() should not take a configuration as part of its argument.

While there is no specification for how to configure a load, the common configuration is defined. By convention though, it seems RequireJS uses require.config() to pass the configuration and the dojo/loader already exposes it as require.config() as well.

Since it isn't required to be AMD compliant, I suggest we drop it, as it makes the API confusing and puts extra run-time code to try to disambiguate the call signatures.

@tomdye
Copy link
Member

tomdye commented Feb 26, 2016

PR raised to add functionality of passing config to plugin load function exposed that when calling require.config(config) followed by require(dependencies), the configure function would be called twice, the second time with an empty object. I think we've got lucky so far that this hasn't caused problems.

@tomdye tomdye self-assigned this Feb 26, 2016
@tomdye
Copy link
Member

tomdye commented Feb 29, 2016

Merge of config still occurs and merge test passes if two calls are made to require.config rather than one to require.config and the second to require with config as the first argument.

On inspection of the requireJS code, it exposes it's require.config function but also accepts config as the first argument to require. In fact, the require.config function simply points at require passing only config. I think it's better if we just remove support for this from our loader. Most apps should be using require.config anyway. - our only loader test that was passing config to require is the aforementioned merge test.

@kitsonk
Copy link
Member Author

kitsonk commented Mar 1, 2016

I suspect accepting config as the first argument was both a legacy for RequireJS and the Dojo loader. It obviously can cause confusion. I would prefer we adhere to the spec, which means it shouldn't be passed as the first argument.

I can't personally thing of a decent use case for it that we wouldn't want to rewrite to make more clear via require.config().

@tomdye
Copy link
Member

tomdye commented Mar 1, 2016

Agreed @kitsonk
PR raised: #50

@kitsonk kitsonk closed this as completed in d014cdc Mar 4, 2016
@dylans dylans added this to the beta.4 milestone Oct 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants