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

Default pattern for migration filenames doesn't match sequelize pattern. #302

Closed
netik opened this issue Aug 25, 2020 · 0 comments · Fixed by #325
Closed

Default pattern for migration filenames doesn't match sequelize pattern. #302

netik opened this issue Aug 25, 2020 · 0 comments · Fixed by #325

Comments

@netik
Copy link

netik commented Aug 25, 2020

One issue we ran into when moving to umzug is that the pattern specified by default doesn't match what sequelize will accept.

For example:

foo.js.js is ignored (but sequelize will accept this)

as is

foo bar.js

The default pattern should match what sequelize accepts, every file in the migratinos directory ending in .js regardless of spaces or double js.

    pattern: /^\d+[\w-]+\.js$/,

in the constructor should probably be:

      pattern: /\.(cjs|js|ts)$/

Which is what is used in migrator.js inside of the sequelize-cli package. Make these the same for consistency.

I realize anyone can change the pattern in the constructor, but it's a silent failure when running migrations unless you know that these migrations are not the same.

If you decide not to implement this, at a minimum update the README so that people know.

@netik netik changed the title Default pattern doesn't match sequelize pattern. Default pattern for migration filenames doesn't match sequelize pattern. Aug 25, 2020
@mmkal mmkal mentioned this issue Oct 1, 2020
5 tasks
@mmkal mmkal closed this as completed in #325 Oct 7, 2020
mmkal added a commit that referenced this issue 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
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant