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

Rename library global from moment-range to MomentRange #152

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

JochenDiekenbrock
Copy link
Contributor

The - character in the library name seems to confuse the browser.

I have added a testcase lib/browser_test.js that checks the expected behaviour as defined in issue 142 (but changed the library name from DateRange to MomentRange because I thought it's more approriate).

To fix the issue, I renamed the library from moment-range to MomentRange. The testcase lib/browser_test.js as well as a privately performed test in the browser showed that
MomentRange.extendMoment(moment);
is working now.

Please pull

karma.conf.js Outdated
@@ -24,6 +24,9 @@ module.exports = function(config) {
},

files: [
{ pattern: 'node_modules/expect.js/index.js', watched: false },
{ pattern: 'node_modules/moment/min/moment.min.js', watched: false },
{ pattern: 'dist/moment-range.js', watched: false },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think it's necessary to list the dependencies in the files key

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't like to add the dependencies, but the test does not work without this.
Of course, I could define a second karma configuration especially for the browser test, if you think it's worth it.

@@ -40,7 +40,7 @@ module.exports = {
},
output: {
filename: 'moment-range.js',
library: 'moment-range',
library: 'MomentRange',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, this'll change the name for all module types. ideally i'd like to keep it as moment-range everywhere except the browser

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you specify file names for the import (ES6) or require (commonJS). In that case, I think the changed name should not effect these cases?
I could not find any option in the webpack configuration to specify the name for the browser case.

@JochenDiekenbrock
Copy link
Contributor Author

I created tests for commonjs and ES6, testing the documented usage for both cases.
Both tests pass, even with the changed library name.
In the ES6 test, I used the fix for issue 153, but that should not be related to the library name.

@Rhysjc Rhysjc added the bugfix label Jul 25, 2017
@gf3
Copy link
Contributor

gf3 commented Dec 20, 2017

this should be merged for the 4.0.0 milestone

@gf3 gf3 added this to the 4.0.0 milestone Dec 20, 2017
@gf3 gf3 added review and removed review labels Jan 26, 2018
@gf3 gf3 changed the title Fix for issue 142 Rename library global from moment-range to MomentRange Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants