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

Compatibility with more AMD loaders (notably curl.js) #1

Merged
merged 2 commits into from
Feb 15, 2012
Merged

Compatibility with more AMD loaders (notably curl.js) #1

merged 2 commits into from
Feb 15, 2012

Conversation

unscriptable
Copy link
Member

I'm just submitting this for review. I just moved the define() call to the bottom to prevent "early export". Ideally, all of the module-ish boilerplate would be together, so I wanted to move it all to the bottom, but then I figured I'd just do something that works and see what you guys had to say. Feedback appreciated! -- John

@ryanflorence
Copy link

👍

The more the merrier. @jrburke, thoughts?

@ryanflorence
Copy link

Also, I'm less concerned about keeping all the exporting boilerplate together than I am about really easy merging w/ upstream and being more inclusive.

@jrburke
Copy link

jrburke commented Feb 4, 2012

I'm OK with this, but my primary concern is getting something that is usable for the upstream repo. We can do this merge in our branch but if some point underscore wants it all together in the same block, I am more amenable to going that route if it means getting the change included in their repo. But we might as well try for the code change we like best.

@unscriptable if that sounds good, @rpflorence I'm +1 on the merge. I assume you will be doing it, but give a holler if you do not have the time -- be sure to generate the min file fresh after the merge and then ideally move the latest version tag to this new version.

@unscriptable
Copy link
Member Author

@jrburke do you think they would be amenable to a boilerplate-at-the bottom approach? That'd work too, of course. -- J

@jrburke
Copy link

jrburke commented Feb 5, 2012

@unscriptable I hope so, that is why I think we should do your pull request and lead with what we think is best and then let their feedback guide any final solution, but just wanted to see how bad it is for you if they decide not to go this route. As of right now I have not gotten any indication they are actually thinking of including an AMD call, just going on the past experience.

@unscriptable
Copy link
Member Author

@jrburke I've got a shim that users can pre-load if the underscore folks decide to do AMD, but insist on keeping the boilerplate at the top. It's not ideal, but it works. -- J

@jrburke
Copy link

jrburke commented Feb 5, 2012

Sounds good let's merge it.

jrburke added a commit that referenced this pull request Feb 15, 2012
Better compatibility with more AMD loaders (notably curl.js)
@jrburke jrburke merged commit f58a0fb into amdjs:master Feb 15, 2012
@jrburke
Copy link

jrburke commented Feb 15, 2012

I merged this in, updated underscore-min.js again for good measure and moved the latest 1.3.1 tag to this merged result.

@unscriptable
Copy link
Member Author

Thanks James!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants