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

Expand SeedsConfig types #3531

Merged
merged 2 commits into from Nov 14, 2019
Merged

Expand SeedsConfig types #3531

merged 2 commits into from Nov 14, 2019

Conversation

@bensaufley
Copy link
Contributor

bensaufley commented Nov 9, 2019

This is not a change to behavior; it's just an update to the TypeScript types to reflect the actuality of what's in the codebase. Inspecting lib/seed/Seeder.js there are five unique references to this.config., but only two were previously represented in the types.

I'm a big fan of generics and how they can help us write more explicit code but if you want to eliminate that for something more broad, like just variables?: object or variables?: any that's fine; I'm not using it.

I'm submitting this PR because I encountered the following issue:

knexfile.ts has the following:

seeds: {
  directory: './db/seeds',
  stub: './db/seed.stub.ts',
}

Using the .ts stub file by way of npx knex seed:make thing generates a migration with a .js extension. Obviously that's not right. I can add -x ts in the command line, but it seems silly to need to do that every time when it's the only way I want to use it in the project.

So I thought, can I add extension as I can with migrations? TypeScript says no. But I tried it, with } as any for the seeds value, and it worked. So then I dug into the code, and yep there it is.

tl;dr SeedsConfig respects extension but the Type doesn't allow it currently. Everything else is secondary.

Since this isn't a change to behavior, and all added keys are optional, it shouldn't be a breaking change and I haven't written any tests. Please let me know if there's more I can do to ensure this meets the standards of the repo.

@lorefnon

This comment has been minimized.

Copy link
Collaborator

lorefnon commented Nov 10, 2019

@bensaufley Thanks for this PR.

I think in addition to this change, we can infer extension from type of knexfile. If knexfile is typescript then in most cases user would want migrations/seeds etc. generated in ts as well. I'll create a new issue for it. That can be taken up separately.

types/index.d.ts Outdated Show resolved Hide resolved
@kibertoad kibertoad merged commit ec9c9b6 into knex:master Nov 14, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 14, 2019

Released in 0.20.2

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