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

Make TS stubs modern #3080

merged 1 commit into from Mar 5, 2019

Conversation

@felixmosh
Copy link
Contributor

@felixmosh felixmosh commented Mar 3, 2019

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

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

@kibertoad kibertoad Mar 3, 2019

Choose a reason for hiding this comment

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

Why remove semicolons, though?

Loading

Copy link
Member

@elhigu elhigu Mar 3, 2019

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.

Loading

Copy link
Collaborator

@kibertoad kibertoad Mar 5, 2019

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?..

Loading

Copy link
Contributor Author

@felixmosh felixmosh Mar 5, 2019

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.

Loading

Copy link
Collaborator

@kibertoad kibertoad Mar 5, 2019

Choose a reason for hiding this comment

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

Fair enough.

Loading

@@ -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

@kibertoad kibertoad Mar 3, 2019

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?

Loading

Copy link
Member

@elhigu elhigu Mar 3, 2019

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.

Loading

Copy link
Collaborator

@kibertoad kibertoad Mar 3, 2019

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

Loading

Copy link
Member

@elhigu elhigu Mar 3, 2019

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 🤔

Loading

Copy link
Collaborator

@kibertoad kibertoad Mar 3, 2019

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.

Loading

@kibertoad kibertoad merged commit f3f0750 into knex:master Mar 5, 2019
2 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants