Skip to content

Commit

Permalink
Ensure Yarn Workspace adapter supports ember try:reset.
Browse files Browse the repository at this point in the history
Fundamentally, `Adapter.prototype.cleanup` **cannot** require
`Adatper.prototype.setup` to have been called. _Most_ of the time
`.setup` is called before `.cleanup` (this happens with `ember try:each`
and `ember try:one`), but when `--skip-cleanup` is specified (e.g.
`ember try:one scenario-name --skip-cleanup`) and you subsequently run
`ember try:reset` the `.setup` happened in a completely different
process.

The issue here is a misunderstanding when we added the
`WorkspaceAdapter` initially. Basically, the assumtion was that `.setup`
was for setting up both local instance state (e.g. the
`this._packageAdapters` array) **and** for actually preparing for a
`ember try:...` run (doing backups / etc).

The fix is to move the internal state setup code directly into
`init`/`constructor, and ensure that `.setup` is only used to create the
backups themselves.
  • Loading branch information
rwjblue committed Mar 12, 2020
1 parent aa4e3ff commit 7648b01
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 14 deletions.
18 changes: 9 additions & 9 deletions lib/dependency-manager-adapters/workspace.js
Expand Up @@ -18,15 +18,6 @@ module.exports = CoreObject.extend({
if (!this.useYarnCommand) {
throw new Error('workspaces are currently only supported by Yarn, you must set `useYarn` to true');
}
},

packageJSON: 'package.json',

setup(options) {
if (!options) {
options = {};
}

let packageJSON = JSON.parse(fs.readFileSync(this.packageJSON));
let workspaceGlobs;

Expand Down Expand Up @@ -59,6 +50,15 @@ module.exports = CoreObject.extend({
});
});

},

packageJSON: 'package.json',

setup(options) {
if (!options) {
options = {};
}

return RSVP.all(this._packageAdapters.map(adapter => adapter.setup(options)));
},

Expand Down
23 changes: 23 additions & 0 deletions test/dependency-manager-adapters/workspace-adapter-test.js
Expand Up @@ -195,6 +195,29 @@ describe('workspaceAdapter', () => {
});

describe('#cleanup', () => {
it('works without having called #setup first', () => {
fs.ensureDirSync('packages/test/node_modules');

writeJSONFile('packages/test/package.json', { originalPackageJSON: false });
writeJSONFile('packages/test/node_modules/prove-it.json', { originalNodeModules: false });

let workspaceAdapter = new WorkspaceAdapter({
cwd: tmpdir,
useYarnCommand: true,
run: () => Promise.resolve(),
});

fs.ensureDirSync('packages/test/.node_modules.ember-try');

writeJSONFile('packages/test/package.json.ember-try', { originalPackageJSON: true });
writeJSONFile('packages/test/.node_modules.ember-try/prove-it.json', { originalNodeModules: true });

return workspaceAdapter.cleanup().then(() => {
assertFileContainsJSON(path.join(tmpdir, 'packages/test/package.json'), { originalPackageJSON: true });
assertFileContainsJSON(path.join(tmpdir, 'packages/test/node_modules/prove-it.json'), { originalNodeModules: true });
});
});

it('replaces the package.json with the backed up version', () => {
fs.ensureDirSync('packages/test/node_modules');

Expand Down
38 changes: 33 additions & 5 deletions test/utils/dependency-manager-adapter-factory-test.js
@@ -1,10 +1,30 @@
'use strict';

const path = require('path');
const fs = require('fs-extra');
const tmp = require('tmp-sync');
const expect = require('chai').expect;
const DependencyManagerAdapterFactory = require('../../lib/utils/dependency-manager-adapter-factory');
const WorkspaceAdapter = require('../../lib/dependency-manager-adapters/workspace');
let writeJSONFile = require('../helpers/write-json-file');

const ROOT = process.cwd();
let tmproot = path.join(ROOT, 'tmp');

describe('DependencyManagerAdapterFactory', () => {
let tmpdir;

beforeEach(() => {
tmpdir = tmp.in(tmproot);
process.chdir(tmpdir);
});

afterEach(() => {
process.chdir(ROOT);

return fs.remove(tmproot);
});

describe('generateFromConfig', () => {
it('creates both adapters, in order, when there is only an npm key', () => {
let adapters = DependencyManagerAdapterFactory.generateFromConfig({ scenarios: [{ npm: {} }] }, 'here');
Expand Down Expand Up @@ -42,11 +62,19 @@ describe('DependencyManagerAdapterFactory', () => {
});

it('creates only a workspace adapter when useWorkspaces is set to true', () => {
let adapters = DependencyManagerAdapterFactory.generateFromConfig({
useYarn: true,
useWorkspaces: true,
scenarios: [{ npm: {} }]
});
writeJSONFile('package.json', { workspaces: ['packages/test'] });

fs.ensureDirSync('packages/test');
writeJSONFile('packages/test/package.json', {});

let adapters = DependencyManagerAdapterFactory.generateFromConfig(
{
useYarn: true,
useWorkspaces: true,
scenarios: [{ npm: {} }]
},
tmpdir
);
expect(adapters[0]).to.be.instanceOf(WorkspaceAdapter);
expect(adapters.length).to.equal(1);
});
Expand Down

0 comments on commit 7648b01

Please sign in to comment.