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

Load isomorphic-ws via import rather than async import #1048

Merged
merged 7 commits into from
Sep 1, 2023

Conversation

brianathere
Copy link
Contributor

@brianathere brianathere commented Aug 22, 2023

PR-Codex overview

Focus of this PR:

This PR focuses on converting the import of isomorphic-ws to a synchronous import.

Detailed summary:

  • Converted isomorphic-ws to a synchronous import in viem patch.
  • Added import statements for WebSocket and MessageEvent in rpc.ts.

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

@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2023

🦋 Changeset detected

Latest commit: 5edec5c

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Aug 22, 2023

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

A member of the Team first needs to authorize it.

@jxom
Copy link
Member

jxom commented Aug 22, 2023

Thanks for the PR! Unfortunately, the sync import of isomorphic-ws breaks in an Edge Runtime environment: #375.

Maybe we can fix this upstream in Node.

@jxom jxom closed this Aug 22, 2023
@brianathere
Copy link
Contributor Author

brianathere commented Aug 22, 2023

@jxom I'm sure that can be fixed here as well. Do you have ay repro steps or environment information for where this is failing?

It really shouldn't be an issue, as the isomorphic code is very simple.

@brianathere
Copy link
Contributor Author

Looking at issues, am I right in guessing it was the vercel edge environment where this issue was encountered?

@brianathere
Copy link
Contributor Author

And is it possible they fixed the issue here: vercel/edge-runtime#309

@jxom jxom reopened this Aug 22, 2023
@jxom
Copy link
Member

jxom commented Aug 22, 2023

Yep, it was Vercel's Edge Runtime. If we can confirm it has been fixed, then am happy to merge this PR!

@brianathere
Copy link
Contributor Author

I'm happy to confirm that, is there a sample app, or test case, I can use? Maybe even just notes on how to reproduce.

@brianathere
Copy link
Contributor Author

I created a sample project here https://github.com/brianathere/viem and published it via vercel. It works fine with my change.

@brianathere
Copy link
Contributor Author

@jxom can you trigger a deploy to vercel to see if anything else causes an issue? I've confirmed with my sample vercel app that viem can be used in the edge runtime, using webscokets, with my change.

brianathere and others added 4 commits September 1, 2023 11:15
Signed-off-by: Brian Meek <brian@hntlabs.com>
Signed-off-by: Brian Meek <brian@hntlabs.com>
@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -9.46% ⚠️

Comparison is base (e37e6ec) 97.40% compared to head (5edec5c) 87.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1048      +/-   ##
==========================================
- Coverage   97.40%   87.95%   -9.46%     
==========================================
  Files         397      386      -11     
  Lines       30780    30356     -424     
  Branches     1908     1436     -472     
==========================================
- Hits        29982    26699    -3283     
- Misses        792     3625    +2833     
- Partials        6       32      +26     
Files Changed Coverage Δ
src/utils/rpc.ts 98.63% <100.00%> (+3.88%) ⬆️

... and 187 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jxom
Copy link
Member

jxom commented Sep 1, 2023

I still seem to be having issues with importing on Edge Runtime:

Screenshot 2023-09-01 at 11 40 54 am

@brianathere
Copy link
Contributor Author

I still seem to be having issues with importing on Edge Runtime:

Screenshot 2023-09-01 at 11 40 54 am

Thank you, where can I see that log? I don't see that in the failing test. I'm happy to fix it if I can figure out the repro.

@jxom
Copy link
Member

jxom commented Sep 1, 2023

Actually, this was probably because we should be using the default export. Retesting now.

@jxom jxom force-pushed the main branch 3 times, most recently from c8cc252 to e37e6ec Compare September 1, 2023 07:43
@jxom jxom merged commit f33086e into wevm:main Sep 1, 2023
9 of 16 checks passed
@github-actions github-actions bot mentioned this pull request Sep 1, 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

Successfully merging this pull request may close these issues.

2 participants