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

Added "type" entry for package exports #759

Merged
merged 1 commit into from
Aug 16, 2022

Conversation

rachaeldawn
Copy link
Contributor

Conventions

Issue: #757

Overview

When moduleResolution is set to NodeNext within a tsconfig.json, it seemingly ignores the legacy types field completely, despite index.d.ts actually being correct in this case. I think it has to do with import specificity, but I can't be certain. The docs say that it looks for a co-located file (effector.mjs -> effector.mjs.d.ts), but it definitely does not work that way, or the file output works in a way that doesn't allow Typescript to resolve the types.

Whatever the problem, I'm not an active contributor to Effector and want to make minimally intrusive changes.

Changes

  • For all .js and .cjs, the exports were updated to have at least a types, require, and default entry.
  • For all .mjs entries, the exports were updated to have a types, import, and default entry
  • For all .umd entries, the exports were updated to have a types, and default.
  • All entries that do not emit a .d.ts are left unchanged

Demo

Please check this link on branch fix-demo. Run npm run demo to prep the repo (Node V16.16.0 + yarn pre-req).

The demo works by using this repo as a git submodule, and file-importing the npm/{package} paths.

Paths imported:

  • effector
  • effector-react
  • effector-solid
  • effector-vue
  • effector-solid
  • forest

Modifying src/index.ts, and using Intellisense for each of the imports in that file, will demonstrate that the types do work. If you would like to contrast this with the broken version, checkout the master branch and try running the npm run build command. The tsc output will complain that it cannot find the types, and Intellisense will also detect nothing.

Risks + Notes

  • In theory, this should not be a breaking change since if it works for people already, it'll simply reference a different declaration file that has the same typings.
  • I confirmed this using CoC.nvim with the coc-tsserver plugin.
  • Further testing may be necessary, but it is unlikely
  • The types entry was filled-in by referencing the node_modules of a new install, and using those specific files as the types value.
  • Double-checking the exports is mind numbing. I strongly recommend reviewing each export carefully. I do not want a production bug because I mis-typed a single field. The demo has a test verifying type exports and default exports exist.

Links

This link is the Typescript manual's overview for the type exports

@netlify
Copy link

netlify bot commented Aug 16, 2022

‼️ Deploy request for effector-docs rejected.

Name Link
🔨 Latest commit 978aca6

@sergeysova
Copy link
Member

@rachaeldawn Thank you for this important fix!
But unfortunately this code will be overwritten, because it’s auto generated.

You need to change this file: https://github.com/effector/effector/blob/master/tools/builder/packages.config.ts

after that call: yarn build and commit it

@rachaeldawn
Copy link
Contributor Author

Hey @sergeysova, this was already done. The package.json changes were performed automatically using yarn build, and not by my hand ☺️

Please reference the changed files, ignoring the package.json files

@rachaeldawn
Copy link
Contributor Author

Thank you, @sergeysova! I'm introducing effector into some project, and this fix is a major help.

I had to copy-paste the types files I need and wrap it in a declare module 'effector' { ... } that is added to the "include" property on the TSConfig for projects that use effector, at least until this is published.

Copy link
Member

@zerobias zerobias left a comment

Choose a reason for hiding this comment

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

Amazing, thanks! I’ll publish new releases at this week (need to check something before that)

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.

3 participants