From 3a93600195c31e7717f0248ea92dcff74da0c9bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andre=CC=81=20Cruz?= Date: Tue, 30 Apr 2013 19:22:47 +0100 Subject: [PATCH] Implement renaming to index in the FsResolver, fix minor issues. --- NOTES.md | 1 + lib/resolve/resolvers/FsResolver.js | 53 ++++++++-- lib/resolve/resolvers/GitResolver.js | 10 +- lib/util/cmd.js | 2 - lib/util/copy.js | 5 +- lib/util/extract.js | 4 + package.json | 4 +- test/assets/package-zip-folder.zip | Bin 322 -> 462 bytes test/assets/package-zip-single-file.zip | Bin 0 -> 170 bytes test/assets/package-zip.zip | Bin 170 -> 968 bytes test/resolve/resolver.js | 23 +++++ test/resolve/resolvers/fsResolver.js | 131 ++++++++++++++++++------ test/resolve/resolvers/gitFsResolver.js | 6 ++ 13 files changed, 186 insertions(+), 53 deletions(-) create mode 100644 test/assets/package-zip-single-file.zip diff --git a/NOTES.md b/NOTES.md index 9408d818..df73abcd 100644 --- a/NOTES.md +++ b/NOTES.md @@ -46,3 +46,4 @@ - add perf tests - url resolver should work with fonts, e.g.: http://fonts.googleapis.com/css?family=Noto+Serif - discuss ability to specify folders inside bower_components.. e.g. components/fonts/ +- discuss namespaces in the registry diff --git a/lib/resolve/resolvers/FsResolver.js b/lib/resolve/resolvers/FsResolver.js index 1118f1a4..c62965bc 100644 --- a/lib/resolve/resolvers/FsResolver.js +++ b/lib/resolve/resolvers/FsResolver.js @@ -23,7 +23,7 @@ mout.object.mixIn(FsResolver, Resolver); FsResolver.prototype.hasNew = function (canonicalPkg) { // If target was specified, simply reject the promise if (this._target !== '*') { - return Q.reject(createError('File system sources can\'t resolve targets ("' + this._target + '")', 'ENORESTARGET')); + return Q.reject(createError('File system sources can\'t resolve targets', 'ENORESTARGET')); } // TODO: should we store latest mtimes in the resolution and compare? @@ -34,12 +34,13 @@ FsResolver.prototype.hasNew = function (canonicalPkg) { FsResolver.prototype._resolveSelf = function () { // If target was specified, simply reject the promise if (this._target !== '*') { - return Q.reject(createError('File system sources can\'t resolve targets ("' + this._target + '")', 'ENORESTARGET')); + return Q.reject(createError('File system sources can\'t resolve targets', 'ENORESTARGET')); } return this._readJson(this._source) .then(this._copy.bind(this)) - .spread(this._extract.bind(this)); + .then(this._extract.bind(this)) + .then(this._rename.bind(this)); }; // ----------------- @@ -51,6 +52,8 @@ FsResolver.prototype._copy = function (meta) { copyOpts, promise; + this._sourceStat = stat; + // Pass in the ignore to the copy options to avoid copying ignored files // Also, pass in the mode to avoid additional stat calls when copying copyOpts = { @@ -68,17 +71,51 @@ FsResolver.prototype._copy = function (meta) { } return promise.then(function () { - return [stat, dstFile || this._tempDir ]; + return dstFile; }.bind(this)); }.bind(this)); }; -FsResolver.prototype._extract = function (stat, file) { - if (stat.isFile() && extract.canExtract(file)) { - return extract(file, this._tempDir); +FsResolver.prototype._extract = function (file) { + if (!file || !extract.canExtract(file)) { + return Q.resolve(); } - return Q.resolve(); + return extract(file, this._tempDir); +}; + +FsResolver.prototype._rename = function () { + // Only rename if original source was a file + if (!this._sourceStat.isFile()) { + return Q.resolve(); + } + + return Q.nfcall(fs.readdir, this._tempDir) + .then(function (files) { + var file, + oldPath, + newPath; + + if (files.length === 1) { + file = files[0]; + this._singleFile = 'index' + path.extname(file); + oldPath = path.join(this._tempDir, file); + newPath = path.join(this._tempDir, this._singleFile); + + return Q.nfcall(fs.rename, oldPath, newPath); + } + }.bind(this)); +}; + +FsResolver.prototype._savePkgMeta = function (meta) { + // TODO: store mtime into the package meta + + // Store main if is a single file + if (this._singleFile) { + meta.main = this._singleFile; + } + + return Resolver.prototype._savePkgMeta.call(this, meta); }; module.exports = FsResolver; \ No newline at end of file diff --git a/lib/resolve/resolvers/GitResolver.js b/lib/resolve/resolvers/GitResolver.js index b00ef328..12ccff34 100644 --- a/lib/resolve/resolvers/GitResolver.js +++ b/lib/resolve/resolvers/GitResolver.js @@ -19,9 +19,7 @@ mout.object.mixIn(GitResolver, Resolver); GitResolver.prototype._resolveSelf = function () { return this._findResolution() - .then(function (resolution) { - this._resolution = resolution; - + .then(function () { return this._checkout() // Always run cleanup after checkout to ensure that .git is removed! // If it's not removed, problems might arrise when the "tmp" module attemps @@ -102,7 +100,7 @@ GitResolver.prototype._findResolution = function (target) { }); } - return { type: 'version', tag: version.tag, commit: version.commit }; + return this._resolution = { type: 'version', tag: version.tag, commit: version.commit }; }.bind(this)); } @@ -111,7 +109,7 @@ GitResolver.prototype._findResolution = function (target) { return self.fetchTags(this._source) .then(function (tags) { if (mout.object.hasOwn(tags, target)) { - return { type: 'tag', tag: target, commit: tags[target] }; + return this._resolution = { type: 'tag', tag: target, commit: tags[target] }; } // Finally check if is a valid branch @@ -134,7 +132,7 @@ GitResolver.prototype._findResolution = function (target) { throw err; } - return { type: 'branch', branch: target, commit: branches[target] }; + return this._resolution = { type: 'branch', branch: target, commit: branches[target] }; }.bind(this)); }.bind(this)); }; diff --git a/lib/util/cmd.js b/lib/util/cmd.js index d775d15d..1d7ff679 100644 --- a/lib/util/cmd.js +++ b/lib/util/cmd.js @@ -22,8 +22,6 @@ function cmd(command, args, options) { var fullCommand, error; - process.removeAllListeners(); - if (code) { // Generate the full command to be presented in the error message if (!Array.isArray(args)) { diff --git a/lib/util/copy.js b/lib/util/copy.js index a1d9f627..0d4d52d8 100644 --- a/lib/util/copy.js +++ b/lib/util/copy.js @@ -9,10 +9,12 @@ function copy(reader, writer) { finish; finish = function (err) { + // Ensure that all listeners are removed writer.removeAllListeners(); reader.removeAllListeners(); // If we got an error, simply reject the deferred + // Otherwise resolve it if (err) { deferred.reject(err); } else { @@ -29,7 +31,8 @@ function copy(reader, writer) { reader = fstream.Reader(reader); } - reader.on('error', finish); + reader + .on('error', finish); // Writer writer = fstream.Writer(writer) diff --git a/lib/util/extract.js b/lib/util/extract.js index 426bee44..51076492 100644 --- a/lib/util/extract.js +++ b/lib/util/extract.js @@ -6,6 +6,10 @@ var tar = require('tar'); var Q = require('q'); var mout = require('mout'); +// This forces the default chunk size to something small in an attempt +// to avoid issue #314 +zlib.Z_DEFAULT_CHUNK = 1024 * 8; + var extractors = { '.zip': extractZip, '.tar': extractTar, diff --git a/package.json b/package.json index 7ef80ced..0cd79bff 100644 --- a/package.json +++ b/package.json @@ -29,14 +29,14 @@ "request": "~2.20.0", "unzip": "~0.1.7", "tar": "~0.1.17", - "walkdir": "0.0.7", "fstream": "~0.1.22", "fstream-ignore": "0.0.6" }, "devDependencies": { "mocha": "~1.8.2", "expect.js": "~0.2.0", - "async": "~0.2.7" + "async": "~0.2.7", + "nock": "~0.17.5" }, "scripts": { "test": "node test/assets/downloader.js && mocha -R spec" diff --git a/test/assets/package-zip-folder.zip b/test/assets/package-zip-folder.zip index 8d4bd753d9c5028fd0f68cdca3217a3f749bfcf4..a4e9256c8cd016b56065152fa7bb1e0394c4ad6f 100644 GIT binary patch literal 462 zcmWIWW@h1H00GA7^PIp8D8a)Zz)+BwoSm4Ss#}#=pdT8+!?209ArOSWGKw$+0CkE0 z)p9VrEbVnVwl;pf0FcKA#6oB~lM;*cvWj8)pC)F4^=o_U1g!|z5E|fr`h56yHWJKB z%g-muJVqu_W?X*bf%=lc;jJTxiRx1hxK9JTL8jpHC&H8^jaF!;z9!&FVrk X3+7{Fvsl?c?q&wUdqDaYh{FH?sB>=R delta 166 zcmX@de29rRz?+$civa|7ub4TJS5bLUPh}toe`OS507;54Z~)=U(q1R9DiASop)B*t zp2~@Z(z5Iv$JWNL7XS+K0dasgBa8RTX<}yJS4I&AZEu~R6#*MU z1N={)58uwl5a7+uacph;dI6wrJ|GV8W@Hj&Mwp5$$Hu?|v%_IYBZvvJg9C0yfHx}} PNQMyz9e{KQh{FH?sOBTY literal 0 HcmV?d00001 diff --git a/test/assets/package-zip.zip b/test/assets/package-zip.zip index a08fd040dd68f68a224241222d23a7cf5f76a036..cb498d01b8ffa87c1497b1dc2a01573cdb5c1260 100644 GIT binary patch literal 968 zcmWIWW@Zs#-~hsxrM*rJNPvw&fFUWdNH426G=hgA?r(kI)5OfcuZ$uL+TJ=rD*`rz z2Kb*oAHJQ9A;6oR*(ws9H9>~HU(%b z2!o6TX~XUmL1b-u@z`CJoFJN-0KozU^t-INCMBRj{^!=B6Xfd1SK^)q(IgTgT_KcCES#BL^9 zfM7R%paX=FNt7AkeB=be#=rwk9zeojNh62}i&74FLJRN)>Bp6f5c>Z*h5+>=B`OYh zqJkKKZ~}6C38FZm8`%g%0z-EV!YJIn@nB?tM>lT$xScWws2}bdy?9vEA{)iZ1`0A( MAdCbmj${Jy04xvr-v9sr delta 68 zcmX@XzKU^z9B*2FzFt;wXao;KPfz7UZSBd?Ogb7cK@N~$;M2s+zyNPnHU=PI1VRTO J9RlJo007yI5gY&j diff --git a/test/resolve/resolver.js b/test/resolve/resolver.js index 11fb920c..e6a210a1 100644 --- a/test/resolve/resolver.js +++ b/test/resolve/resolver.js @@ -75,6 +75,29 @@ describe('Resolver', function () { .done(); }); + it('should throw an error if already resolving', function (next) { + var resolver = new Resolver('foo'), + ok; + + resolver._resolveSelf = function () {}; + + resolver.resolve() + .then(function () { + expect(resolver._resolving).to.not.be.ok(); + return resolver.resolve(); + }) + .then(function () { + next(ok ? null : new Error('Should throw error')); + }) + .done(); + + try { + resolver.resolve(); + } catch (e) { + ok = /already resolving/i.test(e.message); + } + }); + it('should call all the functions necessary to resolve by the correct order', function (next) { function DummyResolver() { Resolver.apply(this, arguments); diff --git a/test/resolve/resolvers/fsResolver.js b/test/resolve/resolvers/fsResolver.js index 266fd2df..b8428030 100644 --- a/test/resolve/resolvers/fsResolver.js +++ b/test/resolve/resolvers/fsResolver.js @@ -46,6 +46,22 @@ describe('FsResolver', function () { }); describe('.hasNew', function () { + it('should fail if a target was specified', function (next) { + var resolver = new FsResolver(testPackage, { + target: '0.0.1' + }); + + resolver.hasNew() + .then(function () { + next(new Error('Should have failed')); + }, function (err) { + expect(err).to.be.an(Error); + expect(err.message).to.match(/can\'t resolve targets/i); + expect(err.code).to.equal('ENORESTARGET'); + next(); + }); + }); + it('should resolve always to true (for now..)', function (next) { var resolver = new FsResolver(path.relative(process.cwd(), testPackage)); @@ -65,16 +81,20 @@ describe('FsResolver', function () { }); describe('.resolve', function () { - it('should copy the source file', function (next) { - var resolver = new FsResolver(path.join(testPackage, 'foo')); + it('should fail if a target was specified', function (next) { + var resolver = new FsResolver(testPackage, { + target: '0.0.1' + }); - resolver.resolve() - .then(function (dir) { - expect(fs.existsSync(path.join(dir, 'foo'))).to.be(true); - expect(fs.existsSync(path.join(dir, 'bar'))).to.be(false); + resolver.hasNew() + .then(function () { + next(new Error('Should have failed')); + }, function (err) { + expect(err).to.be.an(Error); + expect(err.message).to.match(/can\'t resolve targets/i); + expect(err.code).to.equal('ENORESTARGET'); next(); - }) - .done(); + }); }); it('should copy the source directory contents', function (next) { @@ -93,6 +113,55 @@ describe('FsResolver', function () { .done(); }); + it('should copy the source file, renaming it to index', function (next) { + var resolver = new FsResolver(path.join(testPackage, 'foo')); + + resolver.resolve() + .then(function (dir) { + expect(fs.existsSync(path.join(dir, 'index'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'foo'))).to.be(false); + expect(fs.existsSync(path.join(dir, 'bar'))).to.be(false); + }) + .then(function () { + // Test with extension + var resolver = new FsResolver(path.join(testPackage, 'README.md')); + return resolver.resolve(); + }) + .then(function (dir) { + expect(fs.existsSync(path.join(dir, 'index.md'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'README.md'))).to.be(false); + next(); + }) + .done(); + }); + + it.skip('should not rename to index if source is a folder (even with just one file)'); + it.skip('should set the main property in the package metadata if renamed to index'); + + it('should copy the source directory permissions', function (next) { + var mode0777, + resolver; + + tempSource = path.resolve(__dirname, '../../assets/github-test-package-copy'); + resolver = new FsResolver(tempSource); + + copy.copyDir(testPackage, tempSource) + .then(function () { + // Change tempSource dir to 0777 + fs.chmodSync(tempSource, 0777); + // Get the mode to a variable + mode0777 = fs.statSync(tempSource).mode; + }) + .then(resolver.resolve.bind(resolver)) + .then(function (dir) { + // Check if temporary dir is 0777 instead of default 0777 & ~process.umask() + var stat = fs.statSync(dir); + expect(stat.mode).to.equal(mode0777); + next(); + }) + .done(); + }); + it('should copy the source file permissions', function (next) { var mode0777, resolver; @@ -110,31 +179,7 @@ describe('FsResolver', function () { .then(resolver.resolve.bind(resolver)) .then(function (dir) { // Check if file is 0777 - var stat = fs.statSync(path.join(dir, 'temp')); - expect(stat.mode).to.equal(mode0777); - next(); - }) - .done(); - }); - - it('should copy the source directory permissions', function (next) { - var mode0777, - resolver; - - tempSource = path.resolve(__dirname, '../../assets/github-test-package-copy'); - resolver = new FsResolver(tempSource); - - copy.copyDir(testPackage, tempSource) - .then(function () { - // Change tempSource dir to 0777 - fs.chmodSync(tempSource, 0777); - // Get the mode to a variable - mode0777 = fs.statSync(tempSource).mode; - }) - .then(resolver.resolve.bind(resolver)) - .then(function (dir) { - // Check if temporary dir is 0777 instead of default 0777 & ~process.umask() - var stat = fs.statSync(dir); + var stat = fs.statSync(path.join(dir, 'index')); expect(stat.mode).to.equal(mode0777); next(); }) @@ -162,22 +207,40 @@ describe('FsResolver', function () { resolver.resolve() .then(function (dir) { expect(fs.existsSync(path.join(dir, 'foo.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'bar.js'))).to.be(true); expect(fs.existsSync(path.join(dir, 'package-zip.zip'))).to.be(false); next(); }) .done(); }); - it('should copy extracted folder contents if it\'s single to the root', function (next) { + it('should copy extracted folder contents if archive contains only a folder inside', function (next) { var resolver = new FsResolver(path.resolve(__dirname, '../../assets/package-zip-folder.zip')); resolver.resolve() .then(function (dir) { expect(fs.existsSync(path.join(dir, 'foo.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'bar.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'package-zip-folder'))).to.be(false); expect(fs.existsSync(path.join(dir, 'package-zip'))).to.be(false); next(); }) .done(); }); + + + it('should extract if source is an archive and rename to index if it\'s only one file inside', function (next) { + var resolver = new FsResolver(path.resolve(__dirname, '../../assets/package-zip-single-file.zip')); + + resolver.resolve() + .then(function (dir) { + expect(fs.existsSync(path.join(dir, 'index.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'package-zip.zip'))).to.be(false); + next(); + }) + .done(); + }); + + it.skip('should rename single file from a single folder to index when source is an archive'); }); }); \ No newline at end of file diff --git a/test/resolve/resolvers/gitFsResolver.js b/test/resolve/resolvers/gitFsResolver.js index f1777749..09ea12b3 100644 --- a/test/resolve/resolvers/gitFsResolver.js +++ b/test/resolve/resolvers/gitFsResolver.js @@ -31,6 +31,12 @@ describe('GitFsResolver', function () { expect(resolver.getName()).to.equal('github-test-package'); }); + it('should not guess the name from the path if the name was specified', function () { + var resolver = new GitFsResolver(testPackage, { name: 'foo' }); + + expect(resolver.getName()).to.equal('foo'); + }); + it('should make paths absolute and normalized', function () { var resolver;