refactor: replace isomorphic-ws with `unws` #390

sxzz · 2023-04-19T11:05:06Z

unws (https://github.com/sxzz/unws) is designed for modern browsers and bundlers.

isomorphic-ws is a lack of active maintenance, see heineiuo/isomorphic-ws#33

PR-Codex overview

This PR updates the ws package to version 8.12.0 and replaces isomorphic-ws with unws. It also updates the abitype package to version 0.7.1.

Detailed summary

Updated ws package to version 8.12.0
Replaced isomorphic-ws with unws
Updated abitype package to version 0.7.1.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

changeset-bot · 2023-04-19T11:05:09Z

⚠️ No Changeset found

Latest commit: 2daedc3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2023-04-19T11:05:10Z

@sxzz is attempting to deploy a commit to the wagmi Team on Vercel.

A member of the Team first needs to authorize it.

fubhy · 2023-04-19T11:24:49Z

Hm. We are already doing the import dynamically. So why not just do the differentiation ourselve within viem and not rely on a package for that at all?

sxzz · 2023-04-19T11:27:04Z

@fubhy Good idea

sxzz · 2023-04-19T11:45:48Z

@fubhy I tried that. But unfortunately, Webpack cannot be aware that ws will never be imported on the browser. So it will always bundle ws and cause an error.

Module not found: Error: Can't resolve 'zlib' in '/path/viem/node_modules/.pnpm/[email protected]/node_modules/ws/lib'

fubhy · 2023-04-19T12:18:45Z

@fubhy I tried that. But unfortunately, Webpack cannot be aware that ws will never be imported on the browser. So it will always bundle ws and cause an error.
Module not found: Error: Can't resolve 'zlib' in '/path/viem/node_modules/.pnpm/[email protected]/node_modules/ws/lib'

Too bad :-(. Thanks for trying though.

nissoh · 2023-04-19T12:51:43Z

@sxzz @fubhy possibly related, using vite const { WebSocket } = await import('isomorphic-ws') seems to be failing to load on-demand

TypeError: WebSocket is not a constructor will eventually throw.

haven't checked webpack or other bundlers, but i wonder if anyone has some insights on Vite

nissoh · 2023-04-19T13:08:11Z

i've tried to riff around this issue, here's a log coming out of this code in Vite typescript environment with allowSyntheticDefaultImports = true and esModuleInterop = true (just for the kicks)

  const WebSocket2 = await import('isomorphic-ws')
  const { WebSocket } = await import('isomorphic-ws')

  console.log(window.WebSocket) // ƒ WebSocket() { [native code] }
  console.log(WebSocket2) // Module {Symbol(Symbol.toStringTag): 'Module'}
  console.log(WebSocket) // undefined

i was wondering however if const WebSocket = await import('isomorphic-ws') would work on Webpack as-well so both would be compatible? @fubhy

fubhy · 2023-04-19T13:25:07Z

i've tried to riff around this issue, here's a log coming out of this code in Vite typescript environment with allowSyntheticDefaultImports = true and esModuleInterop = true (just for the kicks)
  const WebSocket2 = await import('isomorphic-ws')
  const { WebSocket } = await import('isomorphic-ws')

  console.log(window.WebSocket) // ƒ WebSocket() { [native code] }
  console.log(WebSocket2) // Module {Symbol(Symbol.toStringTag): 'Module'}
  console.log(WebSocket) // undefined
i was wondering however if const WebSocket = await import('isomorphic-ws') would work on Webpack as-well so both would be compatible? @fubhy

Yeah that could be related.

Our vitest test suite runs only in node environment atm. which isn't ideal. I've also discussed with @tmm and @jxom to add consumer acceptance tests by adding a few different example packages with different setups, environments, bundlers, typescript version, etc. and seeing if they compile & run properly.

fubhy · 2023-04-19T13:25:57Z

@nissoh Can you maybe test whether it works for you with the replacement package that @sxzz has proposed?

nissoh · 2023-04-19T13:48:38Z

@nissoh Can you maybe test whether it works for you with the replacement package that @sxzz has proposed?

checked, unws. seems to working properly. package.json includes cjs, esModules and typescript d.ts
https://github.com/sxzz/unws/blob/main/package.json

with that said, i think the core issue is that https://github.com/wagmi-dev/viem/blob/136c4022c3ab4020f37333876bed788a111bff47/src/utils/rpc.ts#L147 doesn't use default export

check https://github.com/heineiuo/isomorphic-ws/blob/master/browser.js
looking at this file, only default is exported. not sure why { Websocket } destrucutre works on webpack

fubhy · 2023-04-19T14:11:39Z

Right. We should probably completely remove the esModuleInterop setting from our tsconfig to guard against this (but not sure if that would have helped here)

sxzz · 2023-04-19T14:17:56Z

but i wonder if anyone has some insights on Vite

Vite or Rollup is an ES module bundler and it will strictly treat the modules. It won't convert or interop the code by default. But webpack will.

All in all, Webpack has more compatibility and Vite respects the specs more (even though some specs are very weird)

fubhy · 2023-04-19T14:18:54Z

but i wonder if anyone has some insights on Vite

Vite or Rollup is an ES module bundler and it will strictly treat the modules. It won't convert or interop the code by default. But webpack will.

All in all, Webpack has more compatibility and Vite respects the specs more (even though some specs are very weird)

Yeah exactly. We should follow the strictest possible setup. 100% agree.

nissoh · 2023-04-19T14:24:00Z

but i wonder if anyone has some insights on Vite

Vite or Rollup is an ES module bundler and it will strictly treat the modules. It won't convert or interop the code by default. But webpack will.
All in all, Webpack has more compatibility and Vite respects the specs more (even though some specs are very weird)

Yeah exactly. We should follow the strictest possible setup. 100% agree.

are we good with @sxzz solution?

can we merge this?

sxzz · 2023-04-19T14:31:17Z

@fubhy seems that esModuleInterop is unnecessary to be disabled. esModuleInterop is enabled by default in the default template of Vite projects.

sxzz · 2023-04-19T14:34:16Z

FYI interopDefault is extremely complicated. See the table https://sokra.github.io/interop-test/

fubhy · 2023-04-19T14:43:04Z

@fubhy seems that esModuleInterop is unnecessary to be disabled. esModuleInterop is enabled by default in the default template of Vite projects.

It might not be necessary for vite projects. I just noticed that it was enabled and it imho shouldn't be. Libraries shouldn't assume any interop whatsoever imho.

FYI interopDefault is extremely complicated. See the table https://sokra.github.io/interop-test/

Yeah. I deep dived into that very topic not long ago as I was bumping a few of my own projects to TS5, NodeNext, etc. and revamped the build setup along the way. Complicated indeed.

sxzz · 2023-04-19T15:26:28Z

@fubhy I just checked. There's no import default or export default in viem codebase. So I guess it won't change anything, so we can safely disable this option. Done.

fubhy · 2023-04-19T15:29:51Z

@fubhy I just checked. There's no import default or export default in viem codebase. So I guess it won't change anything, so we can safely disable this option. Done.

Yeah. It'd basically act as a safe guard to prevent us from introducing sth. like that down the line by accident. I already opened #391 :)

sxzz · 2023-04-19T15:42:31Z

@fubhy Oh I revert my commit.

sxzz · 2023-04-26T03:49:09Z

@jxom Is this PR still needed? I noticed #399 is fixed it.

vercel · 2023-05-03T15:49:27Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
viem	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 3, 2023 3:50pm

codecov · 2023-05-03T16:00:23Z

Codecov Report

Merging #390 (2daedc3) into main (9f71067) will decrease coverage by 4.71%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #390      +/-   ##
==========================================
- Coverage   99.93%   95.22%   -4.71%     
==========================================
  Files         270      267       -3     
  Lines       22974    22335     -639     
  Branches     1963     1748     -215     
==========================================
- Hits        22958    21269    -1689     
- Misses         16     1060    +1044     
- Partials        0        6       +6

Impacted Files	Coverage Δ
src/clients/transports/webSocket.ts	`100.00% <100.00%> (ø)`
src/utils/rpc.ts	`100.00% <100.00%> (ø)`

... and 48 files with indirect coverage changes

jxom · 2023-05-07T22:34:23Z

Let's put this PR on hold for now as isomorphic-ws is working now (at least I think lol). If anymore issues arise, we can consider switching to this!

nissoh · 2023-05-21T10:56:31Z

@sxzz @jxom i'm still having issues transpiling with tsc a standalone project without a bunlder with both esModuleInterop and allowSyntheticDefaultImports turned on

this is reproducible by cloning https://github.com/GMX-Blueberry-Club/Puppet

building by running pnpm run build in @gambitdao-gmx-middleware package would throw

../node_modules/.pnpm/[email protected][email protected]/node_modules/viem/dist/types/utils/rpc.d.ts:2:15 - error TS2595: 'WebSocket' can only be imported by using a default import.

nissoh · 2023-05-30T05:54:24Z

@jxom @fubhy ping fine sers (;

kolirt · 2023-06-23T21:15:03Z

@sxzz @jxom i'm still having issues transpiling with tsc a standalone project without a bunlder with both esModuleInterop and allowSyntheticDefaultImports turned on

this is reproducible by cloning https://github.com/GMX-Blueberry-Club/Puppet

building by running pnpm run build in @gambitdao-gmx-middleware package would throw

../node_modules/.pnpm/[email protected][email protected]/node_modules/viem/dist/types/utils/rpc.d.ts:2:15 - error TS2595: 'WebSocket' can only be imported by using a default import.

same problem. any solutions?

refactor: replace isomorphic-ws with unws

94bc583

jxom force-pushed the main branch from b694a84 to 777fa34 Compare April 24, 2023 02:06

chore: merge

2daedc3

vercel bot deployed to Preview May 3, 2023 15:50 View deployment

jxom closed this May 7, 2023

sxzz deleted the feat/unws branch May 8, 2023 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace isomorphic-ws with `unws` #390

refactor: replace isomorphic-ws with `unws` #390

sxzz commented Apr 19, 2023 •

edited by pr-codex bot

Loading

changeset-bot bot commented Apr 19, 2023 •

edited

Loading

vercel bot commented Apr 19, 2023

fubhy commented Apr 19, 2023

sxzz commented Apr 19, 2023

sxzz commented Apr 19, 2023

fubhy commented Apr 19, 2023

nissoh commented Apr 19, 2023 •

edited

Loading

nissoh commented Apr 19, 2023 •

edited

Loading

fubhy commented Apr 19, 2023

fubhy commented Apr 19, 2023

nissoh commented Apr 19, 2023 •

edited

Loading

fubhy commented Apr 19, 2023 •

edited

Loading

sxzz commented Apr 19, 2023 •

edited

Loading

fubhy commented Apr 19, 2023

nissoh commented Apr 19, 2023

sxzz commented Apr 19, 2023

sxzz commented Apr 19, 2023

fubhy commented Apr 19, 2023

sxzz commented Apr 19, 2023

fubhy commented Apr 19, 2023

sxzz commented Apr 19, 2023

sxzz commented Apr 26, 2023

vercel bot commented May 3, 2023 •

edited

Loading

codecov bot commented May 3, 2023

jxom commented May 7, 2023

nissoh commented May 21, 2023 •

edited

Loading

nissoh commented May 30, 2023

kolirt commented Jun 23, 2023

refactor: replace isomorphic-ws with unws #390

refactor: replace isomorphic-ws with unws #390

Conversation

sxzz commented Apr 19, 2023 • edited by pr-codex bot Loading

PR-Codex overview

Detailed summary

changeset-bot bot commented Apr 19, 2023 • edited Loading

⚠️ No Changeset found

vercel bot commented Apr 19, 2023

fubhy commented Apr 19, 2023

sxzz commented Apr 19, 2023

sxzz commented Apr 19, 2023

fubhy commented Apr 19, 2023

nissoh commented Apr 19, 2023 • edited Loading

nissoh commented Apr 19, 2023 • edited Loading

fubhy commented Apr 19, 2023

fubhy commented Apr 19, 2023

nissoh commented Apr 19, 2023 • edited Loading

fubhy commented Apr 19, 2023 • edited Loading

sxzz commented Apr 19, 2023 • edited Loading

fubhy commented Apr 19, 2023

nissoh commented Apr 19, 2023

sxzz commented Apr 19, 2023

sxzz commented Apr 19, 2023

fubhy commented Apr 19, 2023

sxzz commented Apr 19, 2023

fubhy commented Apr 19, 2023

sxzz commented Apr 19, 2023

sxzz commented Apr 26, 2023

vercel bot commented May 3, 2023 • edited Loading

codecov bot commented May 3, 2023

Codecov Report

jxom commented May 7, 2023

nissoh commented May 21, 2023 • edited Loading

nissoh commented May 30, 2023

kolirt commented Jun 23, 2023

refactor: replace isomorphic-ws with `unws` #390

refactor: replace isomorphic-ws with `unws` #390

sxzz commented Apr 19, 2023 •

edited by pr-codex bot

Loading

changeset-bot bot commented Apr 19, 2023 •

edited

Loading

nissoh commented Apr 19, 2023 •

edited

Loading

nissoh commented Apr 19, 2023 •

edited

Loading

nissoh commented Apr 19, 2023 •

edited

Loading

fubhy commented Apr 19, 2023 •

edited

Loading

sxzz commented Apr 19, 2023 •

edited

Loading

vercel bot commented May 3, 2023 •

edited

Loading

nissoh commented May 21, 2023 •

edited

Loading