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

The Types Are Wrong! #982

Closed
travis opened this issue Oct 19, 2023 · 5 comments
Closed

The Types Are Wrong! #982

travis opened this issue Oct 19, 2023 · 5 comments
Assignees

Comments

@travis
Copy link
Member

travis commented Oct 19, 2023

Plugging a couple of our libraries into https://arethetypeswrong.github.io/ shows that our types could use some improvements:

https://arethetypeswrong.github.io/?p=%40web3-storage%2Fw3up-client%409.0.0
https://arethetypeswrong.github.io/?p=%40web3-storage%2Faccess%409.0.0

Screenshot 2023-10-18 at 4 59 13 PM Screenshot 2023-10-18 at 4 59 01 PM

Fortunately they also have a wonderful key for the errors they show, with a helpful page for fixing each of them:

https://github.com/arethetypeswrong/arethetypeswrong.github.io#readme

We should fix the types on the most forward-facing stuff first because I think this is actively hampering downstream development with our libraries, and it would be good to check all of our libraries soon.

### Tasks
- [ ] https://github.com/web3-storage/w3up/pull/1004
- [ ] https://github.com/web3-storage/w3up/issues/1013
- [ ] https://github.com/web3-storage/w3up/issues/1015
- [ ] https://github.com/web3-storage/w3up/issues/1016
- [ ] https://github.com/web3-storage/w3up/pull/1014
@gobengo
Copy link
Contributor

gobengo commented Oct 24, 2023

@travis will you plz review #1004 ? I think it may resolve this issue.

gobengo added a commit that referenced this issue Oct 25, 2023
Motivation:
* #982

Learned While Devving
* #982 indicated 3 errors coming from atty. Here is how each error type
was resolved
1. `ESM (dynamic import only)` - this is more of a warning than an
error. It's warning, rightly, that versions of node that only use
CommonJS requires (e.g. `require("@web3-storage/did-mailto")`) won't be
able to load these libraries. i.e. these libraries are "ESM-only" in
that they only work on runtimes that can do ESM-style `import()`. By
default, attw warns on this, but this PR configures attw to ignore
instead (via .attw.json
https://github.com/web3-storage/w3up/pull/1004/files#diff-a3be4137698f4bdc5777d3831fac4e67d11c102c5e57dd54906ff333cb551aff
).
2. [internal resolution
error](https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/InternalResolutionError.md)
* Insight via
https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/InternalResolutionError.md#declarations-and-javascript-match-but-are-not-checked-with-correct-settings
> This problem can occur with tsc alone if the library author compiles
with --moduleResolution node (also known as node10) or bundler,
especially in combination with a package.json that has "type": "module"
* This attw error was resolved by this PR but this PR changing all
packages `tsconfig.json` to use moduleResolution=NodeNext (not `Node`).
To do that, many type imports had to change so the imported module
identifier includes a file path suffix
3. [no
types](https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/UntypedResolution.md)
- Most of these were due to the relevant package having sub packabe
submodule listed in package.json `exports` but without a corresponding
js file existing (e.g. it was an old package submodule that has since
been removed)

Also
* added an npm script `attw` to run attw for the package. Now can run
attw for all packages like `pnpm -r attw` from monorepo root
* gitlab ci workflows for the two packages shown in #982 now run attw
@gobengo
Copy link
Contributor

gobengo commented Oct 25, 2023

After the fix from #1004, which affected how npm/pnpm are configured in package.jsons in a way that could affect the release process of our packages, I wanted to do a test release of a package. I just did it with did-mailto and the release went ok and I confirmed the new release of did-mailto resolves arethetypesworking errors as I expected #1013 (comment)

I chose did-mailto because it's the simplest package and in case things went haywire, I think it has the least external dependers.

I'm more worried about the other 3 packages (*-client) that I touched in this PR. https://github.com/web3-storage/w3up/pull/1012/files
Next I'll test one of them

@gobengo
Copy link
Contributor

gobengo commented Oct 25, 2023

The original issue here shows arethetypesworking errors for the two packages of @web3-storage/access and @web3-storage/w3up-client.

I've now released new versions of each and tested that arethetypesworking is happier about them now

@gobengo
Copy link
Contributor

gobengo commented Oct 25, 2023

Here are results of testing the other packages

gobengo added a commit that referenced this issue Oct 25, 2023
…passes due to removal of chain-tracker export (#1014)

Motivation:
* #982
* I was testing the filecoin-client 2.0.1 release I did against attw and
noticed it actually still fails for a tiny reason.
https://arethetypeswrong.github.io/?p=%40web3-storage%2Ffilecoin-client%402.0.1

Change
* now CI for filecoin-client will use attw to prevent releases like this
that dont pass attw check
* also update package.json to resolve the attw update for
filecoin-client
@gobengo
Copy link
Contributor

gobengo commented Oct 25, 2023

@travis would you agree this issue is now resolved?

@travis travis closed this as completed Oct 26, 2023
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

No branches or pull requests

2 participants