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

bug: type inferrence not working fully with NodeNext moduleResolution #244

Closed
1 task done
fubhy opened this issue Mar 23, 2023 · 10 comments
Closed
1 task done

bug: type inferrence not working fully with NodeNext moduleResolution #244

fubhy opened this issue Mar 23, 2023 · 10 comments

Comments

@fubhy
Copy link
Collaborator

fubhy commented Mar 23, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Package Version

0.1.22

Current Behavior

I haven't looked into this further yet but wanted to drop this issue here before I call it a night ...:

When using NodeNext moduleResolution, building with tsc & declaration / declarationMap enabled errors for just about every inferred type that one may try to re-export from viem:

E.g. for something like

export const client = createClient({ ... });

You'd get:

The inferred type of 'client' cannot be named without a reference to '../node_modules/viem/dist/createClient-a28317a9.js'. This is likely not portable. A type annotation is necessary.

Expected Behavior

Viem also works with NodeNext module resolution

Steps To Reproduce

I can set up a reproduction tomorrow and dive deeper & fix this issue, but here are the steps anyways:

  • Set up a pure esm project
  • Re-export sth. from viem, e.g. a client object (createClient(...))
  • Use standard tsc for compilation. Do not use esbuild or swc or any other derived tools like tsup as they generally do not process & honor declaration and declarationMap build output to the same extent (or at all) as tsc
  • Enable declaration and declarationMap output in tsconfig.json
  • Set moduleResolution to NodeNext
  • Run tsc with build output (this is important as this issue doesn't appear with --noEmit or if declaration or declarationMap output is disabled

You'll now see the aforementioned error ...

Link to Minimal Reproducible Example (StackBlitz, CodeSandbox, GitHub repo etc.)

Will share tomorrow (or attempt to submit a fix)

Anything else?

I love the work you are doing to modernize the JS/TS ecosystem in this space. It's long overdue! :-)

Not quite related, but ... I'd really like for viem to also generate (& publish) declarationMap output for better IDE navigation capabilities (same for abitype & wagmi). I understand why tsup has gained this much adoption and I've (and am) using it a lot for side projects too but it can't replace tsc entirely imho, especially for libraries like these!

@fubhy
Copy link
Collaborator Author

fubhy commented Mar 23, 2023

I'd suggest to set up a type & type consumer test suite with different tsconfig configurations and properly build these examples as part of CI to surface errors like these (that only occur when those types are consumed in a downstream library / package). The protobuf-es repository has an interesting setup for this:

We recently added declarationMap & declaration output there too as we ran into the same problem there: bufbuild/protobuf-es#398

@jxom
Copy link
Member

jxom commented Mar 24, 2023

Thanks for the detailed reproduction! Will take a look into it shortly.

@fubhy
Copy link
Collaborator Author

fubhy commented Mar 24, 2023

Thanks for the detailed reproduction! Will take a look into it shortly.

Gm. Alright, sure. I was going to look into this this morning but if you want to go ahead, go ahead :-)

@tmm
Copy link
Member

tmm commented Mar 24, 2023

I would say go for it!

@fubhy
Copy link
Collaborator Author

fubhy commented Mar 27, 2023

Hey, so I've identified a few problems including the cause for this issue.

Before I submit the PR though, I'd like to start a discussion on approach for testing & ensuring downstream compatibility and protection against possible future regressions:

EDIT: I submitted the PR regardless so it's easier to see where I'm coming from (it's a lot of file changes but mostly search & replace)

I was going to add a bit of a test setup for different downstream library & app consumer configurations. I think it would make sense to use the existing (and future) examples and include a build step for them in CI. We could set up a couple of different library consumers in the examples too with different tsconfig configurations. E.g. for CJS & ESM, ESM only, CJS only (if we even still want that, imho we could also make a concious decision to phase out CJS entirely, I think the community is slowly becoming ready for that and with Node v18+ and top-level await in particular there are less and less reasons to still support it).

If you allow me, I'd like to come back to and challenge the current setup with tsup and the custom preconstruct-style dist/src redirect. I can see why that's appealing for local development but it essentially disguises different build & consumer related problems that people will run into downstream without you noticing during development of the library itself. I'd suggest to go with a setup that most closely resembles how the library would be consumed in external downstream apps & libs too. Leveraging the different examples from this repository seems like a good idea.

With the current setup, it's difficult to reproduce this build error.

I'm happy to elaborate further but wanted to test the waters first to see if you'd be open to having the current setup challenged.

@fubhy
Copy link
Collaborator Author

fubhy commented Mar 29, 2023

Gm! I'm keen to help out here as we are currently migrating a bunch of stuff to viem. Any feedback on how to proceed here? :)

@fubhy
Copy link
Collaborator Author

fubhy commented Apr 3, 2023

@fubhy
Copy link
Collaborator Author

fubhy commented Apr 7, 2023

This can be solved now in consuming packages using a type annotation:

export const re: PublicClient = createPublicClient({
  chain: mainnet,
  transport: http(),
});

It's possible that this is not going to be necessary in the future once abitype also supports NodeNext better

@fubhy fubhy closed this as completed Apr 7, 2023
@aliasadolahi74
Copy link

update your typescript version to 5

Copy link
Contributor

github-actions bot commented Jun 1, 2024

This issue has been locked since it has been closed for more than 14 days.

If you found a concrete bug or regression related to it, please open a new bug report with a reproduction against the latest Viem version. If you have any questions or comments you can create a new discussion thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants