feat: glob-based constructor #233
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
papb
reviewed
Jul 5, 2020
papb
reviewed
Jul 5, 2020
papb
requested changes
Jul 5, 2020
papb
reviewed
Jul 5, 2020
| const result = this.options.migrations.wrap(fn).apply(migration, args); | ||
|
|
||
| if (!isPromise(result)) { | ||
| throw new Error(`Migration ${this.file} (or wrapper) didn't return a promise`); |
There was a problem hiding this comment.
Turning this around - why did we need it it in the first place? await 123 and await Promise.resolve(123) both resolve to 123 anyway.
To answer your question, though - removing the check allows passing in jest mocks created with jest.fn() as resolve functions, without having to mock the implementation when we don't really care about it.
There was a problem hiding this comment.
I think this check was here as a safeguard against people forgetting to return their promises in the migration. But maybe you can convince me this shouldn't be our problem...
papb
reviewed
Jul 6, 2020
Co-authored-by: Pedro Augusto de Paula Barbosa <papb1996@gmail.com>
|
Closing in favour of #325 |
mmkal
added a commit
that referenced
this pull request
Oct 7, 2020
Closes #233 (replacement PR) Closes #106 - multiple folders now supported via globbing Closes #169 - shouldn't be as necessary anymore, but potentially further work could be done to make it more convenient to handle many different kinds of migrations Closes #188 - migration class removed Closes #193 - since we're now using `glob`, we can find migrations in symlinked directories. If necessary, in a follow-up we can expose the symlink-related glob options, but there are gotchas there so let's wait for a user request Fixes #302 - `pattern` option is removed in favour of explicit globs Fixes #259 - user is now responsible for globbing/ignoring migration files Closes #185 - although closes as "wontfix" - array support is still gone. `Promise.all` should be used Fixes #171 Touches on #167 - but should continue discussion there as this still doesn't introduce a config option for silently skipping already-applied migrations that are explicitly specified Fixes #33 This is more-or-less a rewrite of the `Umzug` class, consolidating and simplifying several options. Minimal usage now: ```js import { Umzug } from 'umzug' const umzug = new Umzug({ migrations: { glob: 'path/to/migrations/*.js' }, logger: console, }) ``` Note: the `umzug.ts` file is collapsed in GitHub's diff, but that's where the main change is, so it should be opened! TODO: - [x] audit existing issues - many can likely be closed by this - [ ] decide on whether we should make sure consumer is returning a promise. increases test overhead but stops users shooting themselves in the foot. see #233 (comment) - [ ] decide if we want to allow no-oping already-run migrations per #167 - [x] narrow logger interface to only take string messages - can be widened in future, but not narrowed - [ ] document how to support multiple folders. this is a common feature request and globbing supports it via `'{path1/*.js,path2/*.js}'` Summary of the changes, from the readme in the changeset: ___ The Umzug class should be imported as a named import, i.e. `import { Umzug } from 'umzug'`. The `MigrationMeta` type, which is returned by `umzug.executed()` and `umzug.pending()`, no longer has a `file` property - it has a `name` and *optional* `path` - since migrations are not necessarily bound to files on the file system. The `migrations.glob` parameter replaces `path`, `pattern` and `traverseDirectories`. It can be used, in combination with `cwd` and `ignore` to do much more flexible file lookups. See https://npmjs.com/package/glob for more information on the syntax. The `migrations.resolve` parameter replaces `customResolver`. Explicit support for `wrap` and `nameFormatter` has been removed - these can be easily implemented in a `resolve` function. The constructor option `logging` is replaced by `logger` to allow for `warn` and `error` messages in future. NodeJS's global `console` object can be passed to this. To disable logging, replace `logging: false` with `logger: undefined`. The `Umzug#execute` method is removed. Use `Umzug#up` or `Umzug#down`. The options for `Umguz#up` and `Umzug#down` have changed: - `umzug.up({ to: 'some-name' })` and `umzug.down({ to: 'some-name' })` are still valid. - `umzug.up({ from: '...' })` and `umzug.down({ from: '...' })` are no longer supported. To run migrations out-of-order (which is not generally recommended), you can explicitly use `umzug.up({ migrations: ['...'] })` and `umzug.down({ migrations: ['...'] })`. - name matches must be exact. `umzug.up({ to: 'some-n' })` will no longer match a migration called `some-name`. - `umzug.down({ to: 0 })` is still valid but `umzug.up({ to: 0 })` is not. - `umzug.up({ migrations: ['m1', 'm2'] })` is still valid but the shorthand `umzug.up(['m1', 'm2'])` has been removed. - `umzug.down({ migrations: ['m1', 'm2'] })` is still valid but the shorthand `umzug.down(['m1', 'm2'])` has been removed. - `umzug.up({ migrations: ['m1', 'already-run'] })` will throw an error, if `already-run` is not found in the list of pending migrations. - `umzug.down({ migrations: ['m1', 'has-not-been-run'] })` will throw an error, if `has-not-been-run` is not found in the list of executed migrations. - `umzug.up({ migrations: ['m1', 'm2'], force: true })` will re-apply migrations `m1` and `m2` even if they've already been run. - `umzug.down({ migrations: ['m1', 'm2'], force: true })` will "revert" migrations `m1` and `m2` even if they've never been run. - `umzug.up({ migrations: ['m1', 'does-not-exist', 'm2'] })` will throw an error if the migration name is not found. Note that the error will be thrown and no migrations run unless _all_ migration names are found - whether or not `force: true` is added. The `context` parameter replaces `params`, and is passed in as a property to migration functions as an options object, alongs side `name` and `path`. This means the signature for migrations, which in v2 was `(context) => Promise<void>`, has changed slightly in v3, to `({ name, path, context }) => Promise<void>`. The `resolve` function can also be used to upgrade your umzug version to v3 when you have existing v2-compatible migrations: ```js const { Umzug } = require('umzug'); const umzug = new Umzug({ migrations: { glob: 'migrations/umzug-v2-format/*.js', resolve: ({name, path, context}) => { // Adjust the migration from the new signature to the v2 signature, making easier to upgrade to v3 const migration = require(path) return { up: async () => migration.up(context), down: async () => migration.down(context) } } }, context: sequelize.getQueryInterface(), logger: console, }); ``` Similarly, you no longer need `migrationSorting`, you can use `Umzug#extend` to manipulate migration lists directly: ```js const { Umzug } = require('umzug'); const umzug = new Umzug({ migrations: { glob: 'migrations/**/*.js' }, context: sequelize.getQueryInterface(), }) .extend(migrations => migrations.sort((a, b) => b.path.localeCompare(a.path))); ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@papb I've had this on the back-burner for a little while but thought I'd open a PR to start getting feedback. This aims to be the first step to simplifying and replacing the many (and sometimes overlapping) constructor options the current incarnation can take.
README.md,test/umzug.test.tsandtest/sequelize.test.tshave some usage examples, and the readme has some explanations of the "breaking" changes. "breaking" is in quotes because there's nothing removed just yet, it's only an additional function - but if this can go in, we should try out the new API on some known consumers, like @sequelize/cli, and @slonik/migrator, and if we're happy with the changes, the old API should be torn out.