-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
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
feat(core): throw graceful error in runNxMigration() if no collection #10655
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @trakhimenok! Some better messaging in this case would definitely make sense, although it is likely an issue caused by the migration's author.
We recently introduced plugin lint checks that should reduce the likelihood of this occurring, but better messaging still makes sense.
I think try ... catch
may be the wrong control flow for this though, do you care to take a look at my suggestion?
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}"` | ||
); | ||
} |
There was a problem hiding this comment.
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:
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}"` | |
); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AgentEnder addressed
const implRelativePath = g[name]?.implementation || g[name]?.factory; | ||
if (!implRelativePath) { | ||
throw new Error( | ||
`Can not get relative path for migration's implementation as collection not found by name="${name}" for package="${packageName}", collectionPath="${collectionPath}"` |
There was a problem hiding this comment.
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:
`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.
There was a problem hiding this comment.
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.
And better error message
as requested in PR review
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
This will give better context to user when getting an error described in "current behaviour".
Current Behavior
For some unclear reason getting an error when running
pnpm exec nx migrate --run-migrations
Expected Behavior
Expected to get context on where is the problem so might be able to fix.
Related Issue(s)
Fixes #