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

Make TS stubs modern #3080

Merged
merged 1 commit into from Mar 5, 2019
Merged

Conversation

felixmosh
Copy link
Contributor

Small refactor to the TS stubs, so they will use arrow functions and es6 import / export.

t.increments();
t.timestamps();
});
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove semicolons, though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normal function declaration shouldn't have semicolon there. With arrow function, it would be obligatory though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixmosh So if I understand correctly, semicolons should be restored, since it's an arrow function?..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kibertoad , no, up is not an arrow function :]
The function that we pass to createTable is an arrow func, and there are semi-colons there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

@@ -1,9 +1,9 @@
import * as Knex from "knex";

exports.seed = function (knex: Knex): Promise<any> {
export async function seed(knex: Knex): Promise<any> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it returns promise anyway, why make it async?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To allow using await inside the function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Await inside the function is kinda broken, though, not sure we want to encourage it: #2581

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we do :) That is really strange though. It doesn't make sense at all that it works when calling then(), but not with await... Maybe we have missed returning a promise from .then somewhere 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try writing a failing test for that case.

@kibertoad kibertoad merged commit f3f0750 into knex:master Mar 5, 2019
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