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

Added amd support (2 lines) #79

Closed
wants to merge 1 commit into from

Conversation

acornejo
Copy link
Contributor

Added support for loading as AMD module using requirejs.

The additional code required is minimal, and lots of people (including myself) use AMD modules.

This would take care of #72 and #70

@A A added the enhancement label Feb 16, 2014
@A A mentioned this pull request Mar 5, 2014
@nwmcsween
Copy link
Contributor

nack adding project specific code is imo against pagejs style

@nwmcsween nwmcsween closed this Mar 13, 2014
@acornejo
Copy link
Contributor Author

I wouldn't classify this PR as project specific (unless you feel the existing line dedicated to load in node.js is also a project specific part of page.js). There are various other people who opened issues because they needed this feature, and its just 1 line of code (that doesn't break anything).

That being said, its obviously up to you (mantainers).

@Rich-Harris
Copy link

Came here looking for AMD support and found this issue. Seems the easiest solution given the maintainers' desire not to include it is to maintain an AMD-friendly fork, so that's what I've done.

FWIW I'd agree with @acornejo that this isn't project-specific code - AMD is just as much a standard as Node modules (in fact Node modules violate CommonJS Modules/1.1.1 as far as I can tell, by allowing modules to replace the module.exports object). But I also agree with @acornejo that it's entirely the maintainers' decision - ES6 modules will render these arguments moot soon enough anyway ;)

@acornejo
Copy link
Contributor Author

@Rich-Harris I was also forced to maintain a fork. Its available here:

https://github.com/acornejo/page.js

It has not only AMD support, but a lot of other patches which have been proposed here but that never made it to master.

These include:

  • amd support
  • back api support
  • works in IE8 and IE9 (if you include the polyfill)
  • saves path route in context

I am also working on a completely new project which provides all the functionality I miss from page.js, haven't had time to spend on it recently, but hopefully eventually I can complete it and release it as an alternative.

@Rich-Harris
Copy link

@acornejo Ah, nice. Will scrap my fork and use yours.

@nwmcsween
Copy link
Contributor

I am not the only maintainer, others may think differently, also try to
push your patches I will look at some tonight
On 20 Mar 2014 08:04, "Rich Harris" notifications@github.com wrote:

@acornejo https://github.com/acornejo Ah, nice. Will scrap my fork and
use yours.


Reply to this email directly or view it on GitHubhttps://github.com//pull/79#issuecomment-38177731
.

@Rich-Harris
Copy link

Thanks @nwmcsween. My patch would be basically identical to this pull request, so rather than cluttering things up with an identical PR it's probably better that I just add my '+1' here. Appreciate the invitation though.

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.

4 participants