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.

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

Jump to bottom

feat(core): throw graceful error in runNxMigration() if no collection #10655

Merged
merged 4 commits into from
Jun 10, 2022
Merged

feat(core): throw graceful error in runNxMigration() if no collection #10655

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9671350
feat(core): throw graceful error in runNxMigration() if no collection
trakhimenok Jun 9, 2022
120475f
feat(core): check for !value instead of try/catch
trakhimenok Jun 9, 2022
50d309f
feat(core): check for missing collection
trakhimenok Jun 9, 2022
9765504
feat(core): bette error message
trakhimenok Jun 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/nx/src/command-line/migrate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1040,7 +1040,16 @@ async function runNxMigration(root: string, packageName: string, name: string) {

const collection = readJsonFile<MigrationsJson>(collectionPath);
const g = collection.generators || collection.schematics;
const implRelativePath = g[name].implementation || g[name].factory;

let implRelativePath: string;
try {
const c = g[name];
implRelativePath = c.implementation || c.factory;
} catch (e) {
throw new Error(
`Can not get relative path for migration's implementation as collection not found by name="${name}" for package="${packageName}", collectionPath="${collectionPath}"`
Copy link
Member

Choose a reason for hiding this comment

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

This messaging is still not quite accurate. The collection was found, but the implementation was not. I'd recommend something like this:

Suggested change
`Can not get relative path for migration's implementation as collection not found by name="${name}" for package="${packageName}", collectionPath="${collectionPath}"`
`Unable to determine implementation path for ${collectionPath}:${name}`

The collection:generator pattern matches expectations from nx g ..., so I think its more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have a look at new message? I've added also indication if generators or schematics was used.

);
}
Copy link
Member

Choose a reason for hiding this comment

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

Rather than a try catch here, we could do something like this:

Suggested change
try {
const c = g[name];
implRelativePath = c.implementation || c.factory;
} catch (e) {
throw new Error(
`Can not get relative path for migration's implementation as collection not found by name="${name}" for package="${packageName}", collectionPath="${collectionPath}"`
);
}
const c = g[name];
implRelativePath = g[name]?.implementation || g[name]?.factory
if (!implRelativePath) {
throw new Error(
`Unable to determine relative path for migration implementation: { "name": "${name}", "package": "${packageName}", "collectionPath": "${collectionPath}"`
);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AgentEnder addressed


let implPath: string;

Expand Down