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

should not define the module name when doing amd wrapping #20

Closed
normanzb opened this Issue Jan 19, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@normanzb

normanzb commented Jan 19, 2015

The module name should be left to AMD loader implementation to figure out, predefine the module name will cause inconsistency on actual path and virtual path. also confuse AMD loaders specially when "context" setting is enabled.

http://requirejs.org/docs/api.html#config-context

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Jan 20, 2015

Owner

You are talking about this line, right?

define("validate", [], function () { return validate; });

What would be the consequences of simply removing the first argument?

Owner

ansman commented Jan 20, 2015

You are talking about this line, right?

define("validate", [], function () { return validate; });

What would be the consequences of simply removing the first argument?

@normanzb

This comment has been minimized.

Show comment
Hide comment
@normanzb

normanzb Jan 21, 2015

yeah, I think we should remove the first argument so that:

  1. If validate.js is running in non-amd environment, we use global.validate.
  2. If it is running in AMD environment, the developers should knows where the module located and call define(['./path/to/validate'], function(){...}) or require with corresponding path to load it.

By removing the first argument it also benefit below situations:

  1. It doesn't confuse AMD module loader about which context it belongs to.
  2. It doesn't confuse developers on which module name to use, says we place validate.js in a folder named foo, the correct path to load it should be foo/validate, however, because validate.js hard coded module name, above path doesn't work.
  3. After compile it with AMD module optimiser such as r.js, the optimiser can automatically fill in the correct path/module name just after define(. (hard coded module name prevent r.js from filling in the correct module name)

normanzb commented Jan 21, 2015

yeah, I think we should remove the first argument so that:

  1. If validate.js is running in non-amd environment, we use global.validate.
  2. If it is running in AMD environment, the developers should knows where the module located and call define(['./path/to/validate'], function(){...}) or require with corresponding path to load it.

By removing the first argument it also benefit below situations:

  1. It doesn't confuse AMD module loader about which context it belongs to.
  2. It doesn't confuse developers on which module name to use, says we place validate.js in a folder named foo, the correct path to load it should be foo/validate, however, because validate.js hard coded module name, above path doesn't work.
  3. After compile it with AMD module optimiser such as r.js, the optimiser can automatically fill in the correct path/module name just after define(. (hard coded module name prevent r.js from filling in the correct module name)
@normanzb

This comment has been minimized.

Show comment
Hide comment
@normanzb

normanzb Feb 2, 2015

agreed on removing it?

normanzb commented Feb 2, 2015

agreed on removing it?

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Feb 7, 2015

Owner

Would there be any downsides of removing the name?

Owner

ansman commented Feb 7, 2015

Would there be any downsides of removing the name?

@ansman ansman closed this in 79e852f Apr 3, 2015

@ansman ansman added this to the Next milestone Apr 8, 2015

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Apr 8, 2015

Owner

Released in 0.7.0, validate.js no longer supplies a name when calling define

Owner

ansman commented Apr 8, 2015

Released in 0.7.0, validate.js no longer supplies a name when calling define

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