-
Notifications
You must be signed in to change notification settings - Fork 95
Add import readMigrations function in the Database class #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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
export const initial001 = { | ||
up: `CREATE TABLE Category ( | ||
id INTEGER PRIMARY KEY, | ||
name TEXT NOT NULL | ||
); | ||
|
||
CREATE TABLE Post ( | ||
id INTEGER PRIMARY KEY, | ||
categoryId INTEGER NOT NULL, | ||
title TEXT NOT NULL, | ||
isPublished NUMERIC NOT NULL DEFAULT 0, | ||
CONSTRAINT Post_fk_categoryId FOREIGN KEY (categoryId) | ||
REFERENCES Category (id) ON UPDATE CASCADE ON DELETE CASCADE, | ||
CONSTRAINT Post_ck_isPublished CHECK (isPublished IN (0, 1)) | ||
); | ||
|
||
CREATE INDEX Post_ix_categoryId ON Post (categoryId); | ||
|
||
INSERT INTO Category (id, name) VALUES (1, 'Test');`, | ||
down: `DROP INDEX Post_ix_categoryId; | ||
DROP TABLE Post; | ||
DROP TABLE Category;` | ||
} | ||
|
||
export const someFeature002 = { | ||
up: `CREATE TABLE Test ( | ||
id INTEGER PRIMARY KEY, | ||
name TEXT NOT NULL | ||
);`, | ||
down: `DROP TABLE Test;` | ||
} | ||
|
||
export const testCert003 = { | ||
up: `CREATE TABLE whatever ( certificate TEXT ); | ||
INSERT INTO whatever ( certificate ) VALUES ( | ||
'-----BEGIN CERTIFICATE----- | ||
some contents | ||
-----END CERTIFICATE-----');`, | ||
down: `DROP TABLE whatever;` | ||
} | ||
|
||
export const noDown004 = { | ||
up: `CREATE TABLE IF NOT EXISTS downless ( value TEXT ); | ||
INSERT INTO downless ( value ) VALUES ('down migration is optional');` | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,8 +49,11 @@ export async function readMigrations (migrationPath?: string) { | |
return reject(err) | ||
} | ||
|
||
const [up, down] = data.split(/^--\s+?down\b/im) | ||
const [up, down] = data | ||
.split(/^--\s+?down\b/im) | ||
.map(value => value.replace(/^-{3,}$/gim, '').trim()) | ||
|
||
delete migration.filename | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the types consistency. Do you want to save all parameters from migration that may be add in the future? So maybe we will make another type in Migration class for the readMigrations function and in the migrate function we will just require needed arguments but not the exact object as MigrationData. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds appropriate |
||
const migrationData = migration as Partial<MigrationData> | ||
migrationData.up = up.replace(/^-- .*?$/gm, '').trim() // Remove comments | ||
migrationData.down = down ? down.trim() : '' // and trim whitespaces | ||
|
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.
Is the intent to just read a list of migrations and then execute them?
It almost feels like this should belong in its own Migrations class and the constructor would accept a Database instance
then you can have a method for just reading the list of migrations / status
and then have another method(s) for executing up / down based on the list of migrations passed through
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.
Yes, I understand that it's better. But it'll take some time. So let's me finish my tasks and I plan to make this changes on the next week
Changes:
WDYT?
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.
The overall idea sounds good. I'd have to see the impl / interface to have additional thoughts
Thanks for doing this. We could also probably move the migration logic to the migration class too.
The Database instance can still keep the
migrate
method for backwards-compat but would call the new Migration classThere 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.
Hi, I replied a little later but I have time to make changes)