-
Notifications
You must be signed in to change notification settings - Fork 179
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: expose createMigrationBuilder to allow using this lib as sql builder #1285
base: main
Are you sure you want to change the base?
Conversation
25573f2
to
9df3929
Compare
Sorry for the late response, but life... you know What is the purpose of this PR? You wrote about not having file accesses. Potentially we need to go another route and provide an option to not write files and just output to stdio for example 🤔 Also we definitely need tests... |
I want to use node-pg-migrate as dsl builder instead migration tool
|
9df3929
to
b9a601d
Compare
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.
I cannot accept this PR right now, because it exposes something fully new I personally don't want to maintain in the long run. Also it would throw errors on a second run when e.g. a select/query would get executed, like calling for a lock.
We would need to implement it in a totally different like adding a new option parameter to RunnerOption
and then use the logger to do something like a dry-run
.
Line 253 in 2b14645
export async function runner(options: RunnerOption): Promise<RunMigration[]> { |
runner
is getting also called from the CLI, so that would also then be a new option to the CLI.
Would it make sense to expose this under unstable_createMigrationBuilder and return MigrationBuilder instead of MigrationBuilderImpl for now? If a dry-run mode is implemented for the runner, the builder could later incorporate additional methods for SQL generation. Feel free to close this issue if you feel the timing isn’t right. |
fix #1283