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

beforeMigration option #167

Merged
merged 7 commits into from
Jul 30, 2022

Conversation

Ciberusps
Copy link
Contributor

@Ciberusps Ciberusps commented Jul 14, 2022

You can use beforeMigration callback for logging purposes, preparing migration data, etc.
beforeMigration callback will be called before the migration is executed.
It could be useful when u have multiple configs and you want to log the migration data for each config.

My example - I use multiple electron-store's and its usefull to log all migrations via electron-log in single file for debugging purposes, its easier to understand what can cause issue. Probably its can be usefull for others

import Conf from 'conf';

console.log = someLogger.log;

const mainConfig = new Conf({
	beforeMigration: (store, currentVersion, nextVersion) => {
		console.log(`[main-config] migrate from ${currentVersion} -> ${nextVersion}`);
	},
	migrations: {
		'0.4.0': store => {
		   // somethign that might broke config silently
        },
	}
});

const secondConfig = new Conf({
	beforeMigration: (store, currentVersion, nextVersion) => {
		console.log(`[second-config] migrate from ${currentVersion} -> ${nextVersion}`);
	},
	migrations: {
		'1.0.1': store => {
          // somethign that might broke config silently
        },
	}
});

@Ciberusps Ciberusps force-pushed the feature/beforeMigration branch from ebc488f to 20ab166 Compare July 14, 2022 08:54
@Ciberusps Ciberusps marked this pull request as ready for review July 14, 2022 09:05
Repository owner deleted a comment from Piwr Jul 20, 2022
readme.md Outdated
console.log = someLogger.log;

const mainConfig = new Conf({
beforeMigration: (store, currentVersion, nextVersion) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer if it were like this.

Suggested change
beforeMigration: (store, currentVersion, nextVersion) => {
beforeMigration: (store, context) => {

That way we could easily add more metadata properties in the future.

Copy link
Owner

Choose a reason for hiding this comment

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

Should the properties maybe be named fromVersion and toVersion? currentVersion makes it sound like it's the current version of the package, not the current version in the migration process.

Copy link
Owner

Choose a reason for hiding this comment

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

We could maybe also add the package version and all the migration versions. In case the user wants to know where in the migration process they are.

Copy link
Contributor Author

@Ciberusps Ciberusps Jul 20, 2022

Choose a reason for hiding this comment

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

We could maybe also add the package version and all the migration versions. In case the user wants to know where in the migration process they are.

Added context.migrationProcess with finalVersion and migrations not sure how to name them properly probably they can be flattened and lay in context but proper naming required

Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to keep them top-level in context (no nesting).

readme.md Outdated Show resolved Hide resolved
readme.md Outdated

You can use `beforeMigration` callback for logging purposes, preparing migration data, etc.

`beforeMigration` callback will be called before the migration is executed.
Copy link
Owner

Choose a reason for hiding this comment

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

This is ambiguous. Before the whole process or before each step.

readme.md Show resolved Hide resolved
test/index.ts Outdated
cwd: tempy.directory(),
projectVersion: '2.0.0',
beforeMigration: (store, currentVersion, nextVersion) => {
store.set(`beforeMigration ${currentVersion} -> ${nextVersion}`, true);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
store.set(`beforeMigration ${currentVersion} -> ${nextVersion}`, true);
store.set(`beforeMigration ${currentVersion} ${nextVersion}`, true);

@sindresorhus
Copy link
Owner

I think it should be called beforeEachMigration to make it clear it's called for each step and that would also allow adding a beforeMigration hook in the future which would be called only once before all migrations are done.

source/types.ts Outdated
Comment on lines 112 to 113

*/
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
*/
*/

@@ -135,6 +135,11 @@ expectError<number>(config.get('unicorn', 1));

// -- Migrations --
new Conf({
beforeEachMigration: (store, context) => {
console.log(`[main-config] migrate from ${context.fromVersion} → ${context.toVersion}`);
console.log(`[main-config] final migration verison ${context.migrationProcess.finalVersion}, all migrations that were run or will be runned: ${context.migrationProcess.versions.toString()}`);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
console.log(`[main-config] final migration verison ${context.migrationProcess.finalVersion}, all migrations that were run or will be runned: ${context.migrationProcess.versions.toString()}`);
console.log(`[main-config] final migration version ${context.migrationProcess.finalVersion}, all migrations that were run or will be ran: ${context.migrationProcess.versions.toString()}`);

test/index.ts Outdated
}
});

t.is(conf.get('beforeEachMigration 0.0.0 → 1.0.0'), true);
Copy link
Owner

Choose a reason for hiding this comment

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

Use t.true

@Ciberusps Ciberusps force-pushed the feature/beforeMigration branch from 5375e6d to 90da4f5 Compare July 28, 2022 15:30
@Ciberusps Ciberusps requested a review from sindresorhus July 28, 2022 15:31
@sindresorhus sindresorhus merged commit 06262ab into sindresorhus:main Jul 30, 2022
@Ciberusps Ciberusps deleted the feature/beforeMigration branch July 31, 2022 17:50
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.

2 participants