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

feat: npm repo support repository.directory field #163

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/repo.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ const getRepo = async pkg => {
pkgid: pkg
})
}

const info = hostedFromMani(mani)
const url = info ? info.browse() : unknownHostedUrl(rurl)
const url = info ? info.browse(r.directory) : unknownHostedUrl(rurl)

if (!url) {
throw Object.assign(new Error('no repository: could not get url'), {
Expand Down
73 changes: 73 additions & 0 deletions test/tap/repo.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,79 @@ test('npm repo test-repo-url-ssh - non-github (ssh://)', function (t) {
})
})

/* ----- Test by new mock registry: BEGIN ----- */

const Tacks = require('tacks')
const mockTar = require('../util/mock-tarball.js')

const { Dir, File } = Tacks
const testDir = path.join(__dirname, path.basename(__filename, '.js'))

let server
test('setup mocked registry', t => {
common.fakeRegistry.compat({}, (err, s) => {
t.ifError(err, 'registry mocked successfully')
server = s
t.end()
})
})

test('npm repo test-repo-with-directory', t => {
const fixture = new Tacks(Dir({
'package.json': File({})
}))
fixture.create(testDir)
const packument = {
name: 'test-repo-with-directory',
'dist-tags': { latest: '1.2.3' },
versions: {
'1.2.3': {
name: 'test-repo-with-directory',
version: '1.2.3',
dist: {
tarball: `${server.registry}/test-repo-with-directory/-/test-repo-with-directory-1.2.3.tgz`
},
repository: {
type: 'git',
url: 'git+https://github.com/foo/test-repo-with-directory.git',
directory: 'some/directory'
}
}
}
}
server.get('/test-repo-with-directory').reply(200, packument)
return mockTar({
'package.json': JSON.stringify({
name: 'test-repo-with-directory',
version: '1.2.3'
})
}).then(tarball => {
server.get('/test-repo-with-directory/-/test-repo-with-directory-1.2.3.tgz').reply(200, tarball)
return common.npm([
'repo', 'test-repo-with-directory',
'--registry=' + server.registry,
'--loglevel=silent',
'--browser=' + fakeBrowser
])
}).then(([code, stdout, stderr]) => {
t.equal(code, 0)
t.comment(stdout)
t.comment(stderr)

const res = fs.readFileSync(outFile, 'ascii')
t.equal(res, 'https://github.com/foo/test-repo-with-directory/tree/master/some%2Fdirectory\n')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected .../some/directory, not .../some%2Fdirectory. 🤔
But I find this is a problem of hosted-git-info package.
So, I will open a new PR on npm/hosted-git-info. 💪

The relevant code is here:
https://github.com/npm/hosted-git-info/blob/067fd7f3559fc051fd56528f6231a70ec4e634d0/git-host.js#L38-L40

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been fixed! So this test will actually fail now. We'll fix this up when landing this PR! :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikemimik Thanks for your response. I have not touched this PR for a long time, but can I start again?

If you have any advice, I would be glad to accept!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary to start again. The @npm/cli-team triaged this pull-request today and we're accepting it into 6.14.0. We can fix up the test when we pull it into the release, it's no worries :)

You noted that hosted-git-info does some weird url-encoding, and you had to use "%2f". Since then hosted-git-info has had some changes and it's fixed. It now returns properly. The change to the test is just changing %2f back to / :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand it. I'm looking forward to a new release 😊

rimraf.sync(outFile)
})
})

test('cleanup mocked registry', t => {
server.close()
rimraf.sync(testDir)
t.end()
})

/* ----- Test by new mock registry: END ----- */

test('cleanup', function (t) {
fs.unlinkSync(fakeBrowser)
t.pass('cleaned up')
Expand Down