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

Projects
None yet
2 participants
@crallen
Copy link
Contributor

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

This comment has been minimized.

Copy link
Collaborator

commented Jun 2, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

@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 Jun 13, 2019

@kibertoad kibertoad changed the base branch from 0.17 to master Jun 13, 2019

@kibertoad kibertoad merged commit a65a95b into tgriesser:master Jun 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 17, 2019

Released in 0.18.0-next1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.