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

sequelize types import files outside of 'dist' dir, causing downstream tsc issues #14038

Closed
3 of 9 tasks
jdharvey-ibm opened this issue Feb 1, 2022 · 10 comments · Fixed by #14063
Closed
3 of 9 tasks
Assignees
Labels
released type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.

Comments

@jdharvey-ibm
Copy link

jdharvey-ibm commented Feb 1, 2022

Issue Creation Checklist

Bug Description

Typescript builds are failing on projects using sequelize when the typescript project has certain strict typechecking options enabled in its tsconfig.json file.

The root cause is the changes to the types/index.d.ts file introduced in PR #14002 (though there are other similar errors in the index.d.ts file that likely need to be addressed as well).

Additional info:
3f75880
5e93243

This change uses ../ imports to reference files for their types, but since the index.d.ts file ends up top-level in the dist folder during a build, this effectively causes the build output to refer to <module root>/dist/../lib/errors (for example), which resolves to <module root>/lib/errors. This causes unbuilt ts files to be referenced, ultimately resulting in downstream projects failing their tsc builds when settings such as the following are set in their tsconfig.json:

"noImplicitOverride": true,
"strictNullChecks": true,
"exactOptionalPropertyTypes": true,

When all type checking guards available in the tsconfig.json file are turned on, here is the exhaustive list of errors that occur after importing the module into a downstream project:

node_modules/sequelize/lib/errors/aggregate-error.ts:18:3 - error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'BaseError'.

18   toString(): string {
     ~~~~~~~~

node_modules/sequelize/lib/errors/database/exclusion-constraint-error.ts:12:7 - error TS2420: Class 'ExclusionConstraintError' incorrectly implements interface 'ExclusionConstraintErrorOptions'.
  Types of property 'constraint' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.

12 class ExclusionConstraintError extends DatabaseError implements ExclusionConstraintErrorOptions {
         ~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/sequelize/lib/errors/database/exclusion-constraint-error.ts:23:27 - error TS2379: Argument of type '{ stack: string | undefined; }' is not assignable to parameter of type 'ErrorOptions' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
  Types of property 'stack' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.

23     super(options.parent, { stack: options.stack });
                             ~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/sequelize/lib/errors/database/foreign-key-constraint-error.ts:32:27 - error TS2379: Argument of type '{ stack: string | undefined; }' is not assignable to parameter of type 'ErrorOptions' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
  Types of property 'stack' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.

32     super(options.parent, { stack: options.stack });
                             ~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/sequelize/lib/errors/database/unknown-constraint-error.ts:12:7 - error TS2420: Class 'UnknownConstraintError' incorrectly implements interface 'UnknownConstraintErrorOptions'.
  Types of property 'constraint' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.

12 class UnknownConstraintError extends DatabaseError implements UnknownConstraintErrorOptions {
         ~~~~~~~~~~~~~~~~~~~~~~

node_modules/sequelize/lib/errors/database/unknown-constraint-error.ts:23:27 - error TS2379: Argument of type '{ stack: string | undefined; }' is not assignable to parameter of type 'ErrorOptions' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
  Types of property 'stack' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.

23     super(options.parent, { stack: options.stack });
                             ~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/sequelize/lib/errors/validation-error.ts:224:42 - error TS2532: Object is possibly 'undefined'.

224     } else if (this.errors.length > 0 && this.errors[0].message) {
                                             ~~~~~~~~~~~~~~

node_modules/sequelize/lib/errors/validation/unique-constraint-error.ts:31:44 - error TS2379: Argument of type '{ stack: string | undefined; }' is not assignable to parameter of type 'ErrorOptions' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
  Types of property 'stack' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.

31     super(options.message, options.errors, { stack: options.stack });
                                              ~~~~~~~~~~~~~~~~~~~~~~~~


Found 8 errors.

I suspect the type definitions need to be updated to not refer to any files outside of the types source folder.

I can confirm locally that by adjusting these ../ entries to instead be ./ in the dist folder resolves the issues, but I am not knowledgable to know if this translates well to the source files themselves. It looks like it wouldn't work without some additional type files in the types directory.

To recreate yourself:

  1. create a new node project
  2. install typescript
  3. run npx tsc --init
  4. modify the tsconfig.json file to include stricter type checking as per the above
  5. create a basic index.ts file with the following content:
    import { Sequelize } from 'sequelize'
    console.log(Sequelize);
  6. run npx tsc && node src/index.js

Environment

  • Sequelize version: 6.15.0
  • Node.js version: 16.13.2
  • If TypeScript related: 4.5.5

Bug Report Checklist

How does this problem relate to dialects?

  • I think this problem happens regardless of the dialect.
  • I think this problem happens only for the following dialect(s):
  • I don't know, I was using PUT-YOUR-DIALECT-HERE, with connector library version XXX and database version XXX

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, but only if guidance for the ideal solution were provided and if I would not have to go through the extensive setup instructions for running unit tests in the contribution guide, since this issue is not related to actual functionality and is instead a build/packaging issue.
  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I don't know how to start, I would need guidance.
  • No, I don't have the time, although I believe I could do it if I had the time...
  • No, I don't have the time and I wouldn't even know how to start.
@jdharvey-ibm jdharvey-ibm changed the title regression: sequelize types import files outside of 'dist' dir, causing downstream tsc issues sequelize types import files outside of 'dist' dir, causing downstream tsc issues Feb 1, 2022
@WikiRik
Copy link
Member

WikiRik commented Feb 1, 2022

I'm assigning @ephys to this since they have been working on this. About the setup for testing, I think running yarn test-typings (or npm run test-typings) would be good enough for this.

Either way, thanks for reporting this and the extensive description. I think that together we can provide a good solution to this

@WikiRik WikiRik added the type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. label Feb 1, 2022
@jdharvey-ibm
Copy link
Author

Okay cool. I'm definitely happy to help out in any way that I can. I can reliably reproduce it in my local environment, which is great for testing. I just don't want to step on anyone's toes because it does look like there's some work being done to prevent type duplication across the project. I'll see if I can clone the repo and get those test-typing tests running in the mean time.

@ephys
Copy link
Member

ephys commented Feb 1, 2022

This is more issues arrising due to workaround to repair issues with the initial TS migrations :/

I think we could solve this by merging the types & lib folders. We then copy .d.ts files to dist & emit typedefs for ts files.
And then we npmignore /lib & /types

But if we do that we cause issues for users that import directly from /types (they should not do that but I'm sure some do). But that's less important than having the "proper" usage be broken.

@WikiRik
Copy link
Member

WikiRik commented Feb 1, 2022

Other, more radical, option; if this is causing all these issues maybe we should revert the initial TS things for v6? With the changes we've done after it would cause quite some conflicts but it would solve this

@ephys
Copy link
Member

ephys commented Feb 2, 2022

We could transpiles .ts files in /lib to js & copy the emitted typings to /types in the v6 branch. With some manual fixing

@hardzeichyksiarhei
Copy link

Hi! Same problem, project doesn't compile in strict mode.
image

@ephys
Copy link
Member

ephys commented Feb 4, 2022

We'll have a fix soon, I'm merging our existing TS PRs first before changing our build process. Only one left.

@hardzeichyksiarhei
Copy link

We'll have a fix soon, I'm merging our existing TS PRs first before changing our build process. Only one left.

Thanks!:)

@JanFellner
Copy link

JanFellner commented Feb 7, 2022

soon === undefined in typescript 😎 you have a rough timeline for it :)
(otherwise just fixing the typseript issues would heal the pain)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2022

🎉 This issue has been resolved in version 6.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants