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

Use migration.extension from config when generating migration #3242

Merged
merged 3 commits into from Jun 13, 2019

Conversation

crallen
Copy link
Contributor

@crallen crallen commented Jun 1, 2019

In the knex documentation, the config setting "migration.extension" is documented as such:

"extension: the file extension used for the generated migration files (default js)"
source: https://knexjs.org/#Migrations-API

However, this did not actually work as documented. If you were to set this setting to "ts" and then run the "migrate:make" command without the "-x" argument, the migration tool would still generate a "js" migration file.

Upon digging into the CLI code, I discovered that the CLI was not looking for this setting at all. Instead, it would look for an undocumented "ext" setting at the root level of the configuration. Sure enough, setting this in my knexfile (shown below) produced the result I expected.

// knexfile.js - pulls from a knex configuration section in my app
if (process.env.NODE_ENV === 'production') {
  module.exports = require('./dist/config').default.knex;
} else {
  require('ts-node').register();
  const config = require('./src/config').default.knex;
  module.exports = {
    ...config,
    ext: config.migrations.extension
  };
}

This pull request contains a change to the CLI that will add the "migration.extension" setting to the places it looks to determine the extension of the generated migration file, just before checking for this "ext" setting.

@kibertoad
Copy link
Collaborator

@crallen Thank you for your contribution! Could you please add a test for this case?
https://github.com/tgriesser/knex/blob/master/test/cli/knexfile-test.spec.js
is a good example.

@kibertoad
Copy link
Collaborator

@crallen There's an even better example now: https://github.com/tgriesser/knex/blob/master/test/cli/migrate-make.spec.js
Should be pretty straightforward to expand existing tests to cover your case.

@kibertoad
Copy link
Collaborator

@crallen This actually doesn't work. Did you test it? Also documentation is not wrong, extension param is part of Migration API documentation, not CLI one. There is a gap in CLI documentation, however, that is correct.
Fortunately I'm doing changes in this part of the code anyway, so I'll fix your PR along the way.

@kibertoad kibertoad changed the base branch from master to 0.17 June 13, 2019 21:14
@kibertoad kibertoad changed the base branch from 0.17 to master June 13, 2019 21:14
@kibertoad kibertoad merged commit a65a95b into knex:master Jun 13, 2019
@kibertoad
Copy link
Collaborator

Released in 0.18.0-next1

@lookfirst
Copy link

Thanks for adding this, not sure why seed generation was excluded? I'd love to generate my seed files with a .ts extension automatically.

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

3 participants