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

Fix knex d.ts to work with Node16 module mode #5659

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

patrickshipe
Copy link
Contributor

@patrickshipe patrickshipe commented Aug 12, 2023

Currently, the knex type declarations use export default as if it were an ES module. This has not mattered up until recently with Node's increased ESM support and TypeScript's node16 module setting, which is the TypeScript's team recommended setting for Node users.

Bear with me - this is a bit convoluted. Node's behavior when importing CJS from an ES module is to make module.exports the default export. TypeScript adheres to this, which leads to some weird quirks when we look at knex's type declarations. TS properly detects knex as a CJS module, and when it sees a default export in the declarations, it assumes that the knex function is set to module.exports.default (which it also is) but not to module.exports. So to TS, under node16 module mode, the default export from knex is simply an object with a default property. It cannot be called as a function, even though this is perfectly legal at runtime.

What's the fix here? It's quite simple. Use export = syntax to describe the CJS export, and declare the circular references set in knex.js. This works perfectly in both the new world and old, since we are explicitly telling TS the shape of the proper CJS export rather than having it make assumptions.

@patrickshipe patrickshipe changed the title Fix knex d.ts to work with mixed modules Fix knex d.ts to work with Node16 module mode Aug 12, 2023
@patrickshipe
Copy link
Contributor Author

@OlivierCavadenti - would really appreciate a look at this one. The TypeScript team is really pushing Node users to move to module node16 and the current typings make this very difficult. Let me know what else I can do to help!

@patrickshipe
Copy link
Contributor Author

Copy link
Collaborator

@kibertoad kibertoad left a comment

Choose a reason for hiding this comment

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

Thanks! Is it possible to add additional tests which would have been failing under old setup?

@coveralls
Copy link

coveralls commented Sep 11, 2023

Coverage Status

coverage: 92.823%. remained the same when pulling 31cedc2 on patrickshipe:knex-fix-types into 6142d98 on knex:master.

@patrickshipe
Copy link
Contributor Author

@kibertoad I have not found a way to configure the type definition testing tool to test using module node16/nodenext in addition to the current settings. The tool seems to only allow for one tsconfig configuration in the package.json. If someone has a way to do this I am happy to give it a shot.

@kibertoad kibertoad merged commit 2b2cfb4 into knex:master Sep 11, 2023
32 of 34 checks passed
@valtlai
Copy link

valtlai commented Oct 2, 2023

Is there plans to release a new patch version soon that includes this fix?

@kibertoad
Copy link
Collaborator

@valtlai sure, I can publish it tonight

@patrickshipe
Copy link
Contributor Author

@kibertoad will this be published this week? Thank you :)

@Jiralite
Copy link

Jiralite commented Oct 5, 2023

Does this not resolve #5358?

@patrickshipe
Copy link
Contributor Author

It does @Jiralite ! We need a new version published with this code though.

@Jiralite
Copy link

Jiralite commented Oct 5, 2023

Awesome. Yeah, I was more pointing out that issue should ideally be closed then.

@kibertoad
Copy link
Collaborator

Sorry for the delay. 3.0.0 is now out! Please let me know if it works as expected.

@valtlai
Copy link

valtlai commented Oct 6, 2023

@kibertoad Thank you, it’s now working as expected. Edit: this bug is resolved, yeah, but there is now a entirely different bug: #5706.

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.

5 participants