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

fix build error in 'moduleResolution: node' project #3920

Closed
wants to merge 1 commit into from

Conversation

marshalys
Copy link

fix build error in project which config: "moduleResolution": "node" :

node_modules/ethers/types/providers/provider-ipcsocket.d.ts:1:23 - error TS1452: 'resolution-mode' assertions are only supported when `moduleResolution` is `node16` or `nodenext`.

@marshalys
Copy link
Author

fix #3910

@ricmoo ricmoo added on-deck This Enhancement or Bug is currently being worked on. v6 Issues regarding v6 labels Mar 27, 2023
@nissoh
Copy link

nissoh commented Apr 15, 2023

@ricmoo any chance you can review this? can we remove the reference altogether?

@marshalys i think the fix be causing an issue with projects that not configured to work with node's common module resolver

with that said, currently the reference causes issues with the common use of Node module resolution which is often most use-cases

@ricmoo
Copy link
Member

ricmoo commented Apr 15, 2023

I was about to push it out with the last minor push, but got apprehensive at the last minute, worried it might break some existing code.

I want to seek advice, I’ll put it up on Twitter and any TypeScript gurus you know of, I would love their feedback.

I don’t quite understand the nuance of why node16 injects it, but node doesn’t, as I feel node16 should be the more specifying resolution type.

@marshalys
Copy link
Author

@ricmoo any chance you can review this? can we remove the reference altogether?

@marshalys i think the fix be causing an issue with projects that not configured to work with node's common module resolver

with that said, currently the reference causes issues with the common use of Node module resolution which is often most use-cases

I think change tsconfig.types.json file only effect *.d.ts file generate, because tsconfig.types.json extends from tsconfig.base.json witch set moduleResolution to node16, so it will generate d.ts file and add resolution-mode arrtibute to the tag in d.ts files. If project not set moduleResolution to node16 or nodenext will cause this error.
And If you set your project moduleResolution to node16 or nodenext, you may need change some import code, if you import code dynamic。
So I add moduleResolution to node in tsconfig.types.json file to avoid generate resolution-mode attribute.

@ricmoo
Copy link
Member

ricmoo commented Apr 23, 2023

Yes, I agree that it is the only change it makes, but I wanted to make sure that removing that attribute doesn't break some other environment.

I think it is probably safe though, as I've added a CI workflow to try out various configurations.

See: https://github.com/ethers-io/ethers.js/actions/runs/4772139877/jobs/8484486550 :)

@ricmoo
Copy link
Member

ricmoo commented May 20, 2023

Fixed in v6.4.0.

I've also added a bunch of environment tests to check for this regression in the future during the CI.

Thanks! :)

@ricmoo ricmoo closed this May 20, 2023
@ricmoo ricmoo added enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels May 20, 2023
Woodpile37 pushed a commit to Woodpile37/ethers.js that referenced this pull request Jan 14, 2024
Woodpile37 pushed a commit to Woodpile37/ethers.js that referenced this pull request Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants