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

Fix CommonJS export #1771

Closed
wants to merge 1 commit into from
Closed

Conversation

memee
Copy link
Contributor

@memee memee commented Sep 6, 2016

Export line inside factory function had not restrictive enough check that may
caused global namespace polluting problem during Karma test. Karma has a
window.module alias for angular.mock.module method used during tests. And as
a result other libs that may check for CommonJS env could erroneously try to use
require method which is non-existing in browser env. Example:

  if (typeof module !== 'undefined' && module.exports) {
    // CommonJS
    module.exports = factory(require('angular'));
  }

On the other hand this assignment inside factory function was overwritten by
the outcome of factory function itself in following line:

module.exports = factory(require('jquery'), require('moment'));

As a result when we had tried to require('./bootstrap-datetimepicker.js') in
other script file, we got undefined.

@himdel
Copy link

himdel commented Sep 6, 2016

👍, this happens whenever you use angular-mocks, and pollutes module.exports, which in turn causes toastr to try to use require and so on.. :)

Export line inside factory function had not restrictive enough check that may
caused global namespace polluting problem during Karma test. Karma has a
`window.module` alias for `angular.mock.module` method used during tests. And as
a result other libs that may check for CommonJS env could erroneously try to use
`require` method which is non-existing in browser env. Example:

```javascript
  if (typeof module !== 'undefined' && module.exports) {
    // CommonJS
    module.exports = factory(require('angular'));
  }
```

On the other hand this assignment inside factory function was overwritten by
the outcome of factory function itself in following line:

```javascript
module.exports = factory(require('jquery'), require('moment'));
```

As a result when we had tried to `require('./bootstrap-datetimepicker.js')` in
other script file, we got `undefined`.
@Eonasdan
Copy link
Owner

Per the contrib doc, please make your PRs to the development branch. PRs to the master will be closed.

@Eonasdan Eonasdan closed this Oct 27, 2016
Repository owner locked and limited conversation to collaborators Jun 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants