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

Enabling support for git+http #12

Merged
merged 2 commits into from
Feb 6, 2018
Merged

Enabling support for git+http #12

merged 2 commits into from
Feb 6, 2018

Conversation

funkyvisions
Copy link

@funkyvisions funkyvisions commented Jan 29, 2018

Currently Cordova returns a 501 for git+http fetching. This enables the ability to reference git+http. It would be nice if this could be applied to Cordova 7, since we aren't moving to 8 yet. It definitely needs to be in 8 since nofetch is no longer supported. (I assume it's just a matter of updating the package.json dependency in cordova-lib?)

Also, I tried writing a test for this. However the unit tests are actually more like functional tests and hitting real endpoints. I don't have a git+http endpoint to test with that isn't GitHub (the GitHub one is picked up by the hostedGitInfo logic). So, I tested with my internal one that was the root of this problem. I'll post the code here and you can decide how to integrate it into a test (perhaps mocking is in order). This is part of the "test trimID method for npm and git". What I actually want to test is JUST the trim method, so if it was exposed for testing, we wouldn't even have to make the fetch call per se, just verify the trimID function is working properly.

it('should fetch from git+http successfully', function (done) {
        fetch('git+http://git.oclc.org/mobile/oclc-authentication-plugin.git', tmpDir, opts)
            .then(function () {
                // refetch to trigger trimID
                return fetch('git+http://git.oclc.org/mobile/oclc-authentication-plugin.git', tmpDir, opts);
            })
            .then(function (result) {
                var pkgJSON = require(path.join(result, 'package.json'));
                expect(result).toBeDefined();
                expect(fs.existsSync(result)).toBe(true);
                expect(pkgJSON.name).toBe('oclc-authentication-plugin');
            })
            .fail(function (err) {
                console.error(err);
                expect(err).toBeUndefined();
            })
            .fin(done);
    }, 30000);

@funkyvisions
Copy link
Author

No sure how to add reviewers, so I'll just tag them here. @audreyso @stevengill @dpogue
Thanks for taking a look at this.

@dpogue
Copy link
Member

dpogue commented Jan 31, 2018

For the test, you could possibly point at the Apache gitbox mirror of one of the GitHub repos. i.e., git+http://gitbox.apache.org/repos/asf/cordova-plugin-dialogs.git

@funkyvisions
Copy link
Author

@dpogue let me try that and if it works I'll commit the test as well

@stevengill stevengill merged commit f83d822 into apache:master Feb 6, 2018
@stevengill
Copy link
Contributor

Thanks @funkyvisions

@funkyvisions
Copy link
Author

@stevengill You're welcome. What's the chances of this ending up in Cordova 7? We haven't moved to 8 yet because of various reasons. I can muck with my package-lock.json to force this version into Cordova 7, but it would be nice if I didn't have to do that.

@funkyvisions funkyvisions deleted the patch-1 branch February 8, 2018 21:20
brodycj pushed a commit to brodycj/cordova-fetch that referenced this pull request Aug 6, 2018
brodycj pushed a commit to brodycj/cordova-fetch that referenced this pull request Aug 6, 2018
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.

6 participants