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

feat: export hooks for types(v6) #14719

Closed
wants to merge 1 commit into from
Closed

Conversation

Beace
Copy link
Contributor

@Beace Beace commented Jul 5, 2022

Description Of Change

We solved the problem of typescript nodenext in the previous PR.But I found that types/index.d.ts does not export hooks, which will make references such as import { SequelizeHooks } from 'sequelize/types/hooks'; inaccessible in nodenext. Here is a example in sequelize-typescript

import { SequelizeHooks } from 'sequelize/types/hooks';
export interface HookMeta {
    hookType: keyof SequelizeHooks;
    methodName: string;
    options?: HookOptions;
}

image

Todos

  • add export * from './hooks' in index.d.ts
  • upgrade sequelize-typescript, change the import import { SequelizeHooks } from 'sequelize';

@WikiRik WikiRik requested a review from ephys July 5, 2022 08:36
@Beace
Copy link
Contributor Author

Beace commented Jul 9, 2022

Could someone help me look at it? After that, I have to ask PR for serialize-typescript.

Let's import the types in hooks as follows when module is nodenext.

import { SequelizeHooks } from 'sequelize

@Beace Beace changed the title feat: export hooks for types feat: export hooks for types(v6) Jul 9, 2022
@Beace
Copy link
Contributor Author

Beace commented Jul 9, 2022

import { SequelizeHooks } from 'sequelize/types/hooks'; Will throw the error in typescript module nodenext.

https://github.com/sequelize/sequelize-typescript/blob/master/src/hooks/shared/hook-meta.ts

@ephys
Copy link
Member

ephys commented Jul 9, 2022

That's normal, /types is not intended to be used directly

In v6, you can import from /lib instead
In v7, you can import from /_non-semver-use-at-your-own-risk_ instead

If sequelize-typescript is trying to import from /types, using one of the above should fix the issue

I don't want to expose /lib/hooks in the stable api endpoint. It's a bunch of internal code not designed for public use and exposing it means any changes made to it is going to be a breaking change

@Beace
Copy link
Contributor Author

Beace commented Jul 9, 2022

Thanks for your explanation. I understand what you mean, I'm going to give pr to sequelize-typescript.

@Beace
Copy link
Contributor Author

Beace commented Jul 10, 2022

@ephys sequelize/types/hooks -> sequelize/lib/hooks. this will be a breaking update to sequelize-typescript. commonjs is not compatible with esm. The module commonjs cannot found import { SequelizeHooks } from 'sequelize/lib/hooks';

I understand you don't want to expose /lib/hooks in the stable api endpoin, but what the difference between import { SequelizeHooks } from 'sequelize/lib/hooks' and import { SequelizeHooks } from 'sequelize'

@ephys
Copy link
Member

ephys commented Nov 5, 2022

I'm closing this PR because #15123 should have fixed this. Let me know if the issue still exists though

@ephys ephys closed this Nov 5, 2022
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

2 participants