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

Use relative AMD moment dependency #3082

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
5 participants
@dasa
Contributor

dasa commented Mar 31, 2016

Use a relative AMD dependency so that it's not required to have the main defined for the moment package.

@dasa

This comment has been minimized.

Contributor

dasa commented Mar 31, 2016

Please close this PR if #1989 is still valid.

@icambron

This comment has been minimized.

Member

icambron commented Apr 12, 2016

Should this change also include removing the "main" property from package.json? (Honest question - I can never keep track of how all these module loaders work)

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 12, 2016

@icambron well nobody does, that is part of the problem. I don't think we have to remove the main.

@dasa, how is using moment instead of ../moment not using the main property? Also I'm way too worried this is going to break somebody's workflow. When you use relative paths it works no matter where you put it; with this change (loading moment) requires you to have a "global" module moment. If its in libs/ or any other location it won't work.

@dasa

This comment has been minimized.

Contributor

dasa commented Apr 12, 2016

@ichernev - this PR is to change the locales from using moment to ../moment.

@dasa

This comment has been minimized.

Contributor

dasa commented Apr 12, 2016

An alternative solution for avoiding requiring main to be defined could be to use moment/moment, but I'd prefer ../moment if that's acceptable.

@maggiepint

This comment has been minimized.

Member

maggiepint commented Jun 5, 2016

So, I'm not good at module loading stuff, but I want to clean house on pull requests, so I'm wondering, what specific problem does this fix? Like, 'when I am using system.js and I want to do x, I can't do that because y, and this fixes that'.
Without that understanding, it's hard to determine why to make this kind of change, especially since we have concern that it may break the workflow of others.

@dasa

This comment has been minimized.

Contributor

dasa commented Jun 6, 2016

This change is to remove the requirement to have to define a main for the moment package in AMD. See: http://requirejs.org/docs/api.html#packages
If moment is made to be a sibling, then with this change, the user doesn't have to predefine the moment package at all.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 20, 2016

OK, I'm merging this, as requirejs' maintainers suggested.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 20, 2016

Merged in fb9a5c3

@ichernev ichernev closed this Jun 20, 2016

ichernev added a commit that referenced this pull request Jun 20, 2016

Merge pull request #3082 from dasa:amd-moment
Use relative AMD moment dependency

@mj1856 mj1856 added this to the 2.14.0 milestone Jul 5, 2016

@dasa dasa deleted the dasa:amd-moment branch Sep 12, 2016

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