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

Add ts config info to docs #3321

Closed
wants to merge 1 commit into from
Closed

Add ts config info to docs #3321

wants to merge 1 commit into from

Conversation

cgewecke
Copy link
Collaborator

Description

PR adds the current TS config guidelines to the Getting Started section of the docs. At the moment these are in the GitHub README where fewer people might see them.

#2363
#3198
#3215

There are several issues where TS users report difficulty importing Web3 modules because the underlying JS exports like this: (example)

module.exports = Accounts;

but the types don't define a default export. This does not affect people who configure their tsconfig with esModuleInterop: true. This setting is the default in a tsconfig generated with tsc --init.

The issue affects 16 of 20 Web3 modules. It does not affect the main module or utils.

In #3125 @alcuadrado comments:

what is happening right now is that type declarations use named exports, and the actual js files just reassign module.exports. They should use this pattern instead: https://www.typescriptlang.org/docs/handbook/declaration-files/templates/module-class-d-ts.html

...Note that fixing this is a breaking change for TS users

Questions:

  • @joshstevens19, how do you see this issue?
  • @nivida has suggested it might be possible to export in both styles and satisfy everyone. Is this possible or does any change necessarily break one party or another?
  • how problematic is esModuleInterop? Does it cause significant levels of inconvenience or break other common packages for users?

Type of change

  • Documentation

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.424% when pulling 536d700 on docs/add-ts-to-docs into bc27652 on 1.x.

@nivida nivida added 1.x 1.0 related issues Discussion Types Incorrect or missing types labels Jan 23, 2020
@nivida
Copy link
Contributor

nivida commented Jan 23, 2020

@nivida has suggested it might be possible to export in both styles and satisfy everyone.

I didn't suggest it to just satisfy everyone. The idea behind having both export types (default & named) is to not introduce a breaking change for those who have changed their configuration to the documented one. I didn't test it until now how tsc would handle it with both export types. I've also didn't test what happens if we just change it to the correct ones in a project with the documented config. I will definitely do that asap.

@alcuadrado WDYT?

@cgewecke cgewecke closed this Jun 19, 2020
@mpetrunic mpetrunic deleted the docs/add-ts-to-docs branch October 31, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Discussion Types Incorrect or missing types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants