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 dependency/UMD strategy #252

Closed
AurelioDeRosa opened this issue Oct 30, 2015 · 21 comments
Closed

Fix dependency/UMD strategy #252

AurelioDeRosa opened this issue Oct 30, 2015 · 21 comments

Comments

@AurelioDeRosa
Copy link
Member

We should integrate superagent to add support for IE9.

@aendra-rininsland
Copy link
Member

As great as this is and as many things as it will solve, I think this needs to be delayed until 0.10.8 or later; really, it's such a large change that it probably deserves almost a minor version number increment.

@aendra-rininsland aendra-rininsland modified the milestones: v0.10.8, v0.10.7 Nov 11, 2015
@aendra-rininsland aendra-rininsland changed the title Integrate superagent to add support for IE9 Fix dependency/UMD strategy Nov 12, 2015
@aendra-rininsland
Copy link
Member

I'm renaming this because it underlines a larger issue that I hope to address in 0.10.8, namely that we're juggling a boatload of polyfills that have varying levels of support for things like Unicode.

We need:

  • A good XHR/requests library. Open to suggestions, SuperAgent is being considered at the moment, but if we're replacing everything it might make as much sense to use something like Requests.
  • A Unicode-compatible Base64 encoding/decoding library. We shouldn't rely on the browser atob or btoa implementations, stop polyfilling them for unicode or whathaveyou, and just bundle Github.js with a well-supported library that does this sort of thing well.
  • An end to writing our own UMD wrapper. I so so so so so don't want to maintain our own UMD implementation. Browserify and Webpack both compile out to UMD, so this is totally unnecessary.

@AurelioDeRosa
Copy link
Member Author

I agree on all the points. In regard of the third point, I think that we can manage it via Babel and this plugin since we decided to use it.

@aendra-rininsland
Copy link
Member

@AurelioDeRosa Are we rewriting it in ES2015 for 0.10.8? I thought we were leaving that until 0.11.0 — though happy with whatever. Honestly, would only be another 10-15 minutes of work to do so, am pretty ambivalent either way.

@AurelioDeRosa
Copy link
Member Author

No, I was thinking just to integrate the babel system now to have UMD easily. Then, in 1.11.0 we'll write ES6/7.

@clayreimann
Copy link
Member

@AurelioDeRosa are you actively working on this? If not, I'm working on it!

@clayreimann
Copy link
Member

If you're curious you can checkout clayreimann/github#add-webpack.

@AurelioDeRosa
Copy link
Member Author

Hi @clayreimann. Yes, I'm working on it. We decided not to go with yet another build system but to use Babel and its ecosystem to do that. If you want to take a look at what's next, browse the axios branch.

@clayreimann
Copy link
Member

@AurelioDeRosa I presume from the branch name that you're going with axios for a http wrapper. Have you been able to make it work in PhantomJS? I couldn't get it to work.

Also I'll have a PR for you guys later today. I got things working with webpack and did a little brain surgery as opposed to the cosmetic stuff that you guys did in axios. As with any PR feel free to not take it but I think it improves the code quite a bit.

@AurelioDeRosa
Copy link
Member Author

This issue has been fixed and it'll be available in the next release.

@alexisargyris
Copy link

Hello. Any idea when the next release will become available? I am trying to install the current version through jspm and can't get past 'axios.git'.

@AurelioDeRosa
Copy link
Member Author

Hi @alexisargyris.

This should be fixed in version 0.11.2. Are you using that one?

@alexisargyris
Copy link

Yes. I try

jspm install npm:github-api

and I see

Downloading npm:github-api@0.11.2

warn Error on processPackageConfig
     npm dependency format https://github.com/github-tools/axios.git not currently supported by jspm. Post an issue if required.

warn Error processing package config for npm:github-api.

err  Error processing package config for npm:github-api.

Am I doing something wrong?

@AurelioDeRosa
Copy link
Member Author

I think the problem might be with axios, a library we use in our code. So, you should open an issue there.

@alexisargyris
Copy link

If I installed axios independently would github-api installation then succeed?

@alexisargyris
Copy link

So, axios's suggestion essentially was to depend on the axios package from npm instead of a github repo directly (or a fork for that matter). A bit more detail may be found here.

Any ideas on how should I proceed?

@AurelioDeRosa
Copy link
Member Author

So, the problem is that jspm doesn't support depending on a repository directly from GitHub. Honestly, I don't feel this is a problem on our side and this is a feature that should be supported. To clarify why we are using a fork, it's because for backward compatibility we needed to expose the XHR object and axios still doesn't support it. I addressed the issue in this PR but it hasn't been merged yet.

In conclusion, I'd say that the solutions are:

  • jspm needs (if they want) to support packages taken from GitHub
  • axios needs to merge my PR and have a new release. Once done, we can release a patch version to use the official library

I'll keep this issue closed as both the actions needed are not on our side.

@alexisargyris
Copy link

clear enough, thanks

@alexisargyris
Copy link

So, could you please tell me what's the current status? Has your PR been merged (it looks so)? If yes, is there any chance of getting the patch you mentioned above?

@clayreimann
Copy link
Member

clayreimann commented Apr 27, 2016

@alexisargyris v1.0.0 should be out today and will have the fix.

@alexisargyris
Copy link

@clayreimann thanks a lot!

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

No branches or pull requests

4 participants