diff --git a/.jshintrc b/.jshintrc index 52c60909..ba4562d2 100644 --- a/.jshintrc +++ b/.jshintrc @@ -35,7 +35,7 @@ "boss": true, "debug": false, "eqnull": true, - "es5": false, + "es5": true, "esnext": false, "evil": false, "expr": false, diff --git a/README.md b/README.md index 01dff5ea..9c184b76 100644 --- a/README.md +++ b/README.md @@ -199,23 +199,34 @@ Returns the target. Returns the local temporary folder into which the package is being fetched. The files will remain here until the folder is moved when installing. -`Resolver#hasNew(canonicalPackage)`: Promise +`Resolver#hasNew(canonicalPkg)`: Promise -Checks if there is a version more recent than the provided `canonicalPackage` (folder) that complies with the resolver target. +Checks if there is a version more recent than the provided `canonicalPkg` (folder) that complies with the resolver target. +The hasNew process is as follows: + +- Reads the `package meta` from the `canonicalPkg` +- Resolves to `true if it doesn't exist +- Otherwise, calls _hasNew with the `package meta` and the `canonicalPkg` as arguments + +If the resolver is already working, either resolving or checking for a newer version, the promise is immediately +rejected. `Resolver#resolve()`: Promise Resolves the resolver, and returns a promise of a canonical package. The resolve process is as follows: -- calls `_createTempDir()` and waits. -- When done, calls `_resolveSelf()` and waits. +- Calls `_createTempDir()` and waits. +- When done, calls `_resolve()` and waits. - When done, calls `_readJson()` and waits (validation and normalisation also happens here). - When done, calls both functions below, and waits: - `_applyPkgMeta(meta)` - `_savePkgMeta(meta)` - When done, resolves the promise with the *temp dir*, which is now a canonical package. +If the resolver is already working, either resolving or checking for a newer version, the promise is immediately +rejected. + `Resolver#getPkgMeta()`: Object Get the `package meta`. Essentially, it's what you'll find in `.bower.json`. @@ -235,6 +246,11 @@ same source. ##### Protected functions +`Resolver#_hasNew(pkgMeta, canonicalPkg)`: Promise + +The process of checking for a newer version. This function should be as fast as possible. +Concrete resolvers are encouraged to rewrite this function since the default implementation resolves to `true`. + `Resolver#_createTempDir()`: Promise Creates a temporary dir. @@ -261,7 +277,7 @@ Concrete resolvers may override this to add any additional information that migh ##### Abstract functions that must be implemented by concrete resolvers. -`Resolver#_resolveSelf()`: Promise +`Resolver#_resolve()`: Promise The actual process of fetching the package files. This method must he implemented by concrete resolvers. For instance, the `UrlResolver` would download the contents of a URL into the temporary directory in this stage. diff --git a/lib/resolve/Resolver.js b/lib/resolve/Resolver.js index 3c09954e..0759256f 100644 --- a/lib/resolve/Resolver.js +++ b/lib/resolve/Resolver.js @@ -39,25 +39,51 @@ Resolver.prototype.getTempDir = function () { }; Resolver.prototype.hasNew = function (canonicalPkg) { - return Q.resolve(true); + var that = this, + promise, + metaFile; + + // If already working, error out + if (this._working) { + return Q.reject(createError('Already working', 'EWORKING')); + } + + this._working = true; + + // Avoid reading the package meta if _hasNew was not rewritten + if (this._hasNew === Resolver.prototype._hasNew) { + promise = this._hasNew(); + // Otherwise call _hasNew with both the package meta and the canonical package + } else { + metaFile = path.join(canonicalPkg, '.bower.json'); + promise = Q.nfcall(fs.readFile, metaFile) + .then(function (contents) { + var pkgMeta = JSON.parse(contents.toString()); + return that._hasNew(pkgMeta, canonicalPkg); + }, function () { + return true; // Simply resolve to true if there was an error reading the meta + }); + } + + return promise.fin(function () { + that._working = false; + }); }; Resolver.prototype.resolve = function () { var that = this; - // If already resolving, throw an error - // We don't actually reject a promise because this is a - // serious logic error - if (this._resolving) { - throw new Error('Already resolving'); + // If already working, error out + if (this._working) { + return Q.reject(createError('Already working', 'EWORKING')); } - this._resolving = true; + this._working = true; // Create temporary dir return this._createTempDir() // Resolve self - .then(this._resolveSelf.bind(this)) + .then(this._resolve.bind(this)) // Read json, generating the package meta .then(function () { return that._readJson(that._tempDir); @@ -71,14 +97,15 @@ Resolver.prototype.resolve = function () { ]); }) .then(function () { - that._resolving = false; // Resolve with the folder return that._tempDir; }, function (err) { - that._resolving = false; // If something went wrong, unset the temporary dir that._tempDir = null; throw err; + }) + .fin(function () { + that._working = false; }); }; @@ -93,12 +120,16 @@ Resolver.clearRuntimeCache = function () {}; // ----------------- // Abstract function that should be implemented by concrete resolvers -Resolver.prototype._resolveSelf = function () { - throw new Error('_resolveSelf not implemented'); +Resolver.prototype._resolve = function () { + throw new Error('_resolve not implemented'); }; // ----------------- +Resolver.prototype._hasNew = function (pkgMeta, canonicalPkg) { + return Q.resolve(true); +}; + Resolver.prototype._createTempDir = function () { var baseDir = path.join(tmp.tmpdir, 'bower'); diff --git a/lib/resolve/resolvers/FsResolver.js b/lib/resolve/resolvers/FsResolver.js index c62965bc..7085fea2 100644 --- a/lib/resolve/resolvers/FsResolver.js +++ b/lib/resolve/resolvers/FsResolver.js @@ -13,6 +13,11 @@ var FsResolver = function (source, options) { source = path.resolve(source); Resolver.call(this, source, options); + + // If target was specified, simply reject the promise + if (this._target !== '*') { + throw createError('File system sources can\'t resolve targets', 'ENORESTARGET'); + } }; util.inherits(FsResolver, Resolver); @@ -20,23 +25,10 @@ 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', 'ENORESTARGET')); - } - - // TODO: should we store latest mtimes in the resolution and compare? - // this would be beneficial when copying big files/folders - return Q.resolve(true); -}; - -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', 'ENORESTARGET')); - } +// TODO: should we store latest mtimes in the resolution and compare? +// this would be beneficial when copying big files/folders +FsResolver.prototype._resolve = function () { return this._readJson(this._source) .then(this._copy.bind(this)) .then(this._extract.bind(this)) @@ -108,8 +100,6 @@ FsResolver.prototype._rename = function () { }; 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; diff --git a/lib/resolve/resolvers/GitFsResolver.js b/lib/resolve/resolvers/GitFsResolver.js index b2c3e978..0a6a3eb3 100644 --- a/lib/resolve/resolvers/GitFsResolver.js +++ b/lib/resolve/resolvers/GitFsResolver.js @@ -32,7 +32,7 @@ GitFsResolver.prototype._checkout = function () { // Copy files to the temporary directory first return this._copy() .then(cmd.bind(cmd, 'git', ['checkout', '-f', resolution.tag || resolution.branch || resolution.commit], { cwd: this._tempDir })) - // Cleanup unstagged files + // Cleanup unstaged files .then(cmd.bind(cmd, 'git', ['clean', '-f', '-d'], { cwd: this._tempDir })); }; @@ -44,6 +44,7 @@ GitFsResolver.fetchRefs = function (source) { return Q.resolve(this._refs[source]); } + // TODO: should reuse promises! return cmd('git', ['show-ref', '--tags', '--heads'], { cwd : source }) .then(function (stdout) { // Make them an array diff --git a/lib/resolve/resolvers/GitRemoteResolver.js b/lib/resolve/resolvers/GitRemoteResolver.js index c16b87dd..38cb613d 100644 --- a/lib/resolve/resolvers/GitRemoteResolver.js +++ b/lib/resolve/resolvers/GitRemoteResolver.js @@ -42,6 +42,7 @@ GitRemoteResolver.fetchRefs = function (source) { return Q.resolve(this._refs[source]); } + // TODO: should reuse promises! return cmd('git', ['ls-remote', '--tags', '--heads', source]) .then(function (stdout) { // Make them an array diff --git a/lib/resolve/resolvers/GitResolver.js b/lib/resolve/resolvers/GitResolver.js index 12ccff34..86ba8eb5 100644 --- a/lib/resolve/resolvers/GitResolver.js +++ b/lib/resolve/resolvers/GitResolver.js @@ -17,25 +17,10 @@ mout.object.mixIn(GitResolver, Resolver); // ----------------- -GitResolver.prototype._resolveSelf = function () { +GitResolver.prototype._hasNew = function (pkgMeta) { + var oldResolution = pkgMeta._resolution || {}; + return this._findResolution() - .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 - // to delete the temporary folder - .fin(this._cleanup.bind(this)); - }.bind(this)); -}; - -GitResolver.prototype.hasNew = function (canonicalPkg) { - var oldResolution; - - return this._readJson(canonicalPkg) - .then(function (meta) { - oldResolution = meta._resolution || {}; - }) - .then(this._findResolution.bind(this)) .then(function (resolution) { // Check if resolution types are different if (oldResolution.type !== resolution.type) { @@ -52,6 +37,17 @@ GitResolver.prototype.hasNew = function (canonicalPkg) { }); }; +GitResolver.prototype._resolve = function () { + return this._findResolution() + .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 + // to delete the temporary folder + .fin(this._cleanup.bind(this)); + }.bind(this)); +}; + // ----------------- // Abstract functions that should be implemented by concrete git resolvers diff --git a/test/resolve/resolver.js b/test/resolve/resolver.js index e6a210a1..80d0998e 100644 --- a/test/resolve/resolver.js +++ b/test/resolve/resolver.js @@ -48,6 +48,40 @@ describe('Resolver', function () { }); describe('.hasNew', function () { + it('should throw an error if already working (resolving)', function (next) { + var resolver = new Resolver('foo'); + + resolver._resolve = function () {}; + + resolver.resolve(); + + resolver.hasNew() + .then(function () { + next(new Error('Should have failed')); + }, function (err) { + expect(err).to.be.an(Error); + expect(err.code).to.equal('EWORKING'); + expect(err.message).to.match(/already working/i); + next(); + }); + }); + + it('should throw an error if already working (checking for newer version)', function (next) { + var resolver = new Resolver('foo'); + + resolver.hasNew(); + + resolver.hasNew() + .then(function () { + next(new Error('Should have failed')); + }, function (err) { + expect(err).to.be.an(Error); + expect(err.code).to.equal('EWORKING'); + expect(err.message).to.match(/already working/i); + next(); + }); + }); + it('should resolve to true by default', function (next) { var resolver = new Resolver('foo'); @@ -61,7 +95,7 @@ describe('Resolver', function () { }); describe('.resolve', function () { - it('should reject the promise if _resolveSelf is not implemented', function (next) { + it('should reject the promise if _resolve is not implemented', function (next) { var resolver = new Resolver('foo'); resolver.resolve() @@ -69,33 +103,46 @@ describe('Resolver', function () { next(new Error('Should have rejected the promise')); }, function (err) { expect(err).to.be.an(Error); - expect(err.message).to.contain('_resolveSelf not implemented'); + expect(err.message).to.contain('_resolve not implemented'); next(); }) .done(); }); - it('should throw an error if already resolving', function (next) { - var resolver = new Resolver('foo'), - ok; + it('should throw an error if already working (resolving)', function (next) { + var resolver = new Resolver('foo'); - resolver._resolveSelf = function () {}; + resolver._resolve = function () {}; + + resolver.resolve(); 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(); + next(new Error('Should have failed')); + }, function (err) { + expect(err).to.be.an(Error); + expect(err.code).to.equal('EWORKING'); + expect(err.message).to.match(/already working/i); + next(); + }); + }); - try { - resolver.resolve(); - } catch (e) { - ok = /already resolving/i.test(e.message); - } + it('should throw an error if already working (checking newer version)', function (next) { + var resolver = new Resolver('foo'); + + resolver._resolve = function () {}; + + resolver.hasNew(); + + resolver.resolve() + .then(function () { + next(new Error('Should have failed')); + }, function (err) { + expect(err).to.be.an(Error); + expect(err.code).to.equal('EWORKING'); + expect(err.message).to.match(/already working/i); + next(); + }); }); it('should call all the functions necessary to resolve by the correct order', function (next) { @@ -123,7 +170,7 @@ describe('Resolver', function () { return val; }.bind(this)); }; - DummyResolver.prototype._resolveSelf = function () {}; + DummyResolver.prototype._resolve = function () {}; DummyResolver.prototype._readJson = function () { this._stack.push('before _readJson'); return Resolver.prototype._readJson.apply(this, arguments) @@ -172,7 +219,7 @@ describe('Resolver', function () { it('should resolve with the canonical package (folder)', function (next) { var resolver = new Resolver('foo'); - resolver._resolveSelf = function () {}; + resolver._resolve = function () {}; resolver.resolve() .then(function (folder) { @@ -195,7 +242,7 @@ describe('Resolver', function () { it('should still return null', function (next) { var resolver = new Resolver('foo'); - resolver._resolveSelf = function () { + resolver._resolve = function () { throw new Error('I\'ve failed to resolve'); }; @@ -210,7 +257,7 @@ describe('Resolver', function () { it('should return the canonical package (folder) if resolve succeeded', function (next) { var resolver = new Resolver('foo'); - resolver._resolveSelf = function () {}; + resolver._resolve = function () {}; resolver.resolve() .then(function () { @@ -235,7 +282,7 @@ describe('Resolver', function () { it('should still return null', function (next) { var resolver = new Resolver('foo'); - resolver._resolveSelf = function () { + resolver._resolve = function () { throw new Error('I\'ve failed to resolve'); }; @@ -250,7 +297,7 @@ describe('Resolver', function () { it('should return the package meta if resolve succeeded', function (next) { var resolver = new Resolver('foo'); - resolver._resolveSelf = function () {}; + resolver._resolve = function () {}; resolver.resolve() .then(function () { diff --git a/test/resolve/resolvers/fsResolver.js b/test/resolve/resolvers/fsResolver.js index b8428030..5a38fb1f 100644 --- a/test/resolve/resolvers/fsResolver.js +++ b/test/resolve/resolvers/fsResolver.js @@ -43,25 +43,26 @@ describe('FsResolver', function () { resolver = new FsResolver(testPackage + '/something/..'); expect(resolver.getSource()).to.equal(testPackage); }); - }); - describe('.hasNew', function () { - it('should fail if a target was specified', function (next) { - var resolver = new FsResolver(testPackage, { - target: '0.0.1' - }); + it('should error out if a target was specified', function (next) { + var resolver; - resolver.hasNew() - .then(function () { - next(new Error('Should have failed')); - }, function (err) { + try { + resolver = new FsResolver(testPackage, { + target: '0.0.1' + }); + } catch (err) { expect(err).to.be.an(Error); expect(err.message).to.match(/can\'t resolve targets/i); expect(err.code).to.equal('ENORESTARGET'); - next(); - }); - }); + return next(); + } + next(new Error('Should have thrown')); + }); + }); + + describe('.hasNew', function () { it('should resolve always to true (for now..)', function (next) { var resolver = new FsResolver(path.relative(process.cwd(), testPackage)); @@ -73,30 +74,14 @@ describe('FsResolver', function () { .done(); }); - it.skip('should be false if the file mtime hasn\'t changed'); - it.skip('should be false if the directory mtime hasn\'t changed'); - it.skip('should be true if the file mtime has changed'); - it.skip('should be true if the directory mtime has changed'); - it.skip('should ignore files specified to be ignored'); + //it.skip('should be false if the file mtime hasn\'t changed'); + //it.skip('should be false if the directory mtime hasn\'t changed'); + //it.skip('should be true if the file mtime has changed'); + //it.skip('should be true if the directory mtime has changed'); + //it.skip('should ignore files specified to be ignored'); }); describe('.resolve', 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 copy the source directory contents', function (next) { var resolver = new FsResolver(testPackage); diff --git a/test/resolve/resolvers/gitFsResolver.js b/test/resolve/resolvers/gitFsResolver.js index 09ea12b3..28d3ed64 100644 --- a/test/resolve/resolvers/gitFsResolver.js +++ b/test/resolve/resolvers/gitFsResolver.js @@ -244,5 +244,7 @@ describe('GitFsResolver', function () { }) .done(); }); + + it.skip('should reuse promises for the same source, avoiding making duplicate fetchs'); }); }); \ No newline at end of file diff --git a/test/resolve/resolvers/gitRemoteResolver.js b/test/resolve/resolvers/gitRemoteResolver.js index 851ba203..0ee6b62f 100644 --- a/test/resolve/resolvers/gitRemoteResolver.js +++ b/test/resolve/resolvers/gitRemoteResolver.js @@ -153,5 +153,7 @@ describe('GitRemoteResolver', function () { }) .done(); }); + + it.skip('should reuse promises for the same source, avoiding making duplicate fetchs'); }); }); \ No newline at end of file diff --git a/test/resolve/resolvers/gitResolver.js b/test/resolve/resolvers/gitResolver.js index 9b53ab75..025d30c4 100644 --- a/test/resolve/resolvers/gitResolver.js +++ b/test/resolve/resolvers/gitResolver.js @@ -32,7 +32,7 @@ describe('GitResolver', function () { it('should be true when the resolution type is different', function (next) { var resolver; - fs.writeFileSync(path.join(tempDir, 'bower.json'), JSON.stringify({ + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ name: 'foo', version: '0.0.0', _resolution: { @@ -59,7 +59,7 @@ describe('GitResolver', function () { it('should be true when a higher version for a range is available', function (next) { var resolver; - fs.writeFileSync(path.join(tempDir, 'bower.json'), JSON.stringify({ + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ name: 'foo', version: '1.0.0', _resolution: { @@ -88,7 +88,7 @@ describe('GitResolver', function () { it('should be true when a resolved to a lower version of a range', function (next) { var resolver; - fs.writeFileSync(path.join(tempDir, 'bower.json'), JSON.stringify({ + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ name: 'foo', version: '1.0.1', _resolution: { @@ -116,7 +116,7 @@ describe('GitResolver', function () { it('should be false when resolved to the same tag (with same commit hash) for a given range', function (next) { var resolver; - fs.writeFileSync(path.join(tempDir, 'bower.json'), JSON.stringify({ + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ name: 'foo', version: '1.0.1', _resolution: { @@ -145,7 +145,7 @@ describe('GitResolver', function () { it('should be true when resolved to the same tag (with different commit hash) for a given range', function (next) { var resolver; - fs.writeFileSync(path.join(tempDir, 'bower.json'), JSON.stringify({ + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ name: 'foo', version: '1.0.1', _resolution: { @@ -174,7 +174,7 @@ describe('GitResolver', function () { it('should be true when a different commit hash for a given branch is available', function (next) { var resolver; - fs.writeFileSync(path.join(tempDir, 'bower.json'), JSON.stringify({ + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ name: 'foo', _resolution: { type: 'branch', @@ -200,7 +200,7 @@ describe('GitResolver', function () { it('should be false when resolved to the the same commit hash for a given branch', function (next) { var resolver; - fs.writeFileSync(path.join(tempDir, 'bower.json'), JSON.stringify({ + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ name: 'foo', _resolution: { type: 'branch', @@ -226,7 +226,7 @@ describe('GitResolver', function () { it('should be false when targeting commit hashes', function (next) { var resolver; - fs.writeFileSync(path.join(tempDir, 'bower.json'), JSON.stringify({ + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ name: 'foo', _resolution: { type: 'commit', @@ -249,7 +249,7 @@ describe('GitResolver', function () { }); }); - describe('._resolveSelf', function () { + describe('._resolve', function () { afterEach(clearResolverRuntimeCache); it('should call the necessary functions by the correct order', function (next) {