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.

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

Add down-to command, update dev dependencies #4

Merged
merged 2 commits into from May 25, 2023
Merged

Add down-to command, update dev dependencies #4

merged 2 commits into from May 25, 2023

Conversation

theogravity
Copy link
Contributor

This adds a down-to command, which calls migrator.migrateTo() to migrate down to a specified migration name (or no migrations entirely).

Also updated the dev deps to the latest kysley and typescript version and fixed related TS issues.

@@ -29,15 +33,19 @@ Please specify DATABASE_URL to run this CLi. Try the following:

const db = new Kysely({
dialect: new PostgresDialect({
connectionString: DATABASE_URL,
pool: new Pool({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the newer version of the pg dialect uses a pool instance to specify the connection string

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks!


if (name === 'NO_MIGRATIONS') {
console.log(`Migrating all the way down`)
results = await migrator.migrateTo(NO_MIGRATIONS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the NO_MIGRATIONS constant value is not a string, so it has to be imported directly from kysely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, just realized kysely is a dev dep, maybe we should just copy the NO_MIGRATIONS const value to this file and so it can remain as such:

const NO_MIGRATIONS: NoMigrations = freeze({ __noMigrations__: true })

Copy link
Owner

Choose a reason for hiding this comment

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

Users will install kysely on their own, so I think the current code is okay

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not 100% sure what NO_MIGRATIONS does, even though reading the doc 😄

https://github.com/kysely-org/kysely/blob/dd0319a8854e8e74c1b797354e1109b7bd95602e/src/migration/migrator.ts#L151-L158

However, it is officially supported by Kysely so I'll make this as is.

@@ -1,7 +1,7 @@
import fs from 'fs'

import { program } from 'commander'
import { Kysely, MigrationResultSet, Migrator } from 'kysely'
import {Kysely, MigrationResultSet, Migrator, NO_MIGRATIONS} from 'kysely'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

might want to do import type to prevent non-types from being imported such as NO_MIGRATIONS (see note below)

Copy link
Owner

@acro5piano acro5piano left a comment

Choose a reason for hiding this comment

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

Thank you! Just a minor type level fix is required to merge.

@@ -29,15 +33,19 @@ Please specify DATABASE_URL to run this CLi. Try the following:

const db = new Kysely({
dialect: new PostgresDialect({
connectionString: DATABASE_URL,
pool: new Pool({
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks!


if (name === 'NO_MIGRATIONS') {
console.log(`Migrating all the way down`)
results = await migrator.migrateTo(NO_MIGRATIONS)
Copy link
Owner

Choose a reason for hiding this comment

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

Users will install kysely on their own, so I think the current code is okay


if (name === 'NO_MIGRATIONS') {
console.log(`Migrating all the way down`)
results = await migrator.migrateTo(NO_MIGRATIONS)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not 100% sure what NO_MIGRATIONS does, even though reading the doc 😄

https://github.com/kysely-org/kysely/blob/dd0319a8854e8e74c1b797354e1109b7bd95602e/src/migration/migrator.ts#L151-L158

However, it is officially supported by Kysely so I'll make this as is.

src/run.ts Outdated
.argument('<migration-name>')
.description('Migrates down to the specified migration name. Specify "NO_MIGRATIONS" to migrate all the way down.')
.action(async (name) => {
let results
Copy link
Owner

Choose a reason for hiding this comment

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

Please type the variable

Suggested change
let results
let results: MigrationResultSet

@acro5piano
Copy link
Owner

Also I should state what cli.js offers. It was experimental code and conflicts the README. After this PR get merged, I'll updtae the README.

@acro5piano
Copy link
Owner

@theogravity Thanks!

@acro5piano acro5piano merged commit 52024ed into acro5piano:main May 25, 2023
@acro5piano
Copy link
Owner

Released on v0.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants