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

Check for CommonJS 'require' not needed in UMD wrapper #1810

Closed
jgoz opened this issue Jun 15, 2015 · 3 comments
Closed

Check for CommonJS 'require' not needed in UMD wrapper #1810

jgoz opened this issue Jun 15, 2015 · 3 comments

Comments

@jgoz
Copy link

jgoz commented Jun 15, 2015

The current UMD wrapper checks that all of require, module, and exports have been defined in order to export knockout as a CommonJS module (relevant line).

This causes a problem with some module bundlers (such as webpack) because require may not be defined as a function if a module has no dependencies, as is the case with knockout. If require is not defined, knockout's UMD logic will fall through to the 'global' export method rather than CommonJS.

Since require is not actually used by knockout (again, no dependencies) it is unnecessary to check for its existence; module and exports should be sufficient to detect CommonJS.

@brianmhunt
Copy link
Member

I am under the impression that we would be going for returnExportsGlobal style wrapper. Or are we aiming for (is there a reason to aim for) something more lodashy?

Incidentally, as @jgoz points out, it's okay to not check require; the jQuery library only checks module and exports.

@mbest mbest added this to the 3.4.0 milestone Jun 15, 2015
@jgoz
Copy link
Author

jgoz commented Jun 15, 2015

@brianmhunt

I am under the impression that we would be going for returnExportsGlobal style wrapper. Or are we aiming for (is there a reason to aim for) something more lodashy?

I'm not familiar with all of knockout's supported execution environments, but I would think the 'simplified' returnExports would be a closer match to what you have now, while still resolving this issue.

@brianmhunt
Copy link
Member

@jgoz Right you are. Thank you.

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