Skip to content

fix(cli): handle missing target artifact dir #4112

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.

Sign up for GitHub

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

Merged
raymondfeng merged 1 commit into from
Nov 25, 2019
Merged

fix(cli): handle missing target artifact dir #4112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 14 additions & 6 deletions packages/cli/lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,21 @@ exports.findArtifactPaths = async function(dir, artifactType, reader) {
const readdir = reader || readdirAsync;
debug(`Finding artifact paths at: ${dir}`);

// Wrapping readdir in case it's not a promise.
const files = await readdir(dir);
return _.filter(files, f => {
return (
_.endsWith(f, `${artifactType}.js`) || _.endsWith(f, `${artifactType}.ts`)
try {
// Wrapping readdir in case it's not a promise.
const files = await readdir(dir);
return files.filter(
f =>
_.endsWith(f, `${artifactType}.js`) ||
_.endsWith(f, `${artifactType}.ts`),
);
});
} catch (err) {
if (err.code === 'ENOENT') {
Copy link
Contributor

@raymondfeng raymondfeng Nov 12, 2019

Choose a reason for hiding this comment

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

I would prefer to check if the directory exists in advance instead of using the err code.

Copy link
Member Author

@bajtos bajtos Nov 14, 2019

Choose a reason for hiding this comment

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

I find that approach problematic, because it introduces a race condition - between the time we check if the directory exists, and the time we read the entries, the directory can be deleted or removed.

What are your concerns about my proposed approach?

Copy link
Contributor

@raymondfeng raymondfeng Nov 15, 2019

Choose a reason for hiding this comment

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

I don't think we need to deal with the edge case where the directory is removed when the command in running. If so, we'll have to use sync fs apis.

My main concern is to rely on the error code. It would be more authoritative by calling an api to check existence of the directory.

It's not a blocker for the PR, anyway.

// Target directory was not found (e.g. "src/models" does not exist yet).
return [];
Copy link
Member

@dhmlau dhmlau Nov 25, 2019

Choose a reason for hiding this comment

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

Question: I assume if the target directory does not exist, it will get created when creating the artifact?

Copy link
Member Author

@bajtos bajtos Nov 25, 2019

Choose a reason for hiding this comment

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

Yes, that's correct.

}
throw err;
}
};
/**
* Parses the files of the target directory and returns matching JavaScript
Expand Down
8 changes: 2 additions & 6 deletions packages/cli/test/integration/generators/controller.integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,7 @@ describe('lb4 controller', () => {
}),
)
.withPrompts(restCLIInputComplete),
).to.be.rejectedWith(
/ENOENT: no such file or directory, scandir(.*?)models\b/,
);
).to.be.rejectedWith(/No models found in .*[\/\\]models\b/);
});

it('fails when no repository directory present', () => {
Expand All @@ -230,9 +228,7 @@ describe('lb4 controller', () => {
}),
)
.withPrompts(restCLIInputComplete),
).to.be.rejectedWith(
/ENOENT: no such file or directory, scandir(.*?)repositories\b/,
);
).to.be.rejectedWith(/No repositories found in .*[\/\\]repositories\b/);
});
});
});
Expand Down
16 changes: 16 additions & 0 deletions packages/cli/test/integration/generators/model.integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const path = require('path');
const assert = require('yeoman-assert');
const {expect, TestSandbox} = require('@loopback/testlab');
const {expectFileToMatchSnapshot} = require('../../snapshots');
const {remove} = require('fs-extra');

const generator = path.join(__dirname, '../../../generators/model');
const tests = require('../lib/artifact-generator')(generator);
Expand Down Expand Up @@ -118,6 +119,21 @@ describe('lb4 model integration', () => {
basicModelFileChecks(expectedModelFile, expectedIndexFile);
});

it('creates "src/models" directory if it does not exist', async () => {
await testUtils
.executeGenerator(generator)
.inDir(SANDBOX_PATH, async () => {
testUtils.givenLBProject(SANDBOX_PATH);
await remove(path.resolve(SANDBOX_PATH, 'src/models'));
})
.withPrompts({
name: 'test',
propName: null,
});

basicModelFileChecks(expectedModelFile, expectedIndexFile);
});

it('scaffolds correct files with model base class', async () => {
await testUtils
.executeGenerator(generator)
Expand Down