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

polyfill buffer for WalletConnect on RainbowKit #166

Merged
merged 6 commits into from Aug 30, 2022

Conversation

scottrepreneur
Copy link
Collaborator

@scottrepreneur scottrepreneur commented Aug 16, 2022

Linear Ticket

Fixes PRO-388, Looks like we need to polyfill this for Vite.

Description

RainbowKit requires some polyfills for Vite.

Status

  • walletconnect working with polyfill

@scottrepreneur scottrepreneur marked this pull request as ready for review August 23, 2022 16:01
…/polyfill-buffer

# Conflicts:
#	apps/protocol-frontend/vite.config.ts
#	yarn.lock
@scottrepreneur
Copy link
Collaborator Author

Not sure why the action is failing where it is however.

@jonathanprozzi
Copy link
Collaborator

Not sure why the action is failing where it is however.

This is a strange spot to fail. Going to look into it shortly -- did it build fine locally?

@scottrepreneur
Copy link
Collaborator Author

build works for me locally

delete submodule
@scottrepreneur
Copy link
Collaborator Author

There was a govrn-monorepo nested submodule?? Seems weird, but works w/o

@@ -15,6 +15,7 @@
"@chakra-ui/react": "^1.8.8",
"@emotion/react": "^11.9.3",
"@emotion/styled": "^11.9.3",
"@esbuild-plugins/node-globals-polyfill": "^0.1.1",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to grab your attention that this lib is not well maintained and have some issues.
You can find more here

https://github.com/remorses/esbuild-plugins/blob/63b1ee1f47c7b0deb46bd71ec13d4a538e443d4d/node-modules-polyfill/src/index.ts#L39-L44

Simply put: it just paste couple of imports to every file which can lead to complex implications.

CC: @alexkeating @jonathanprozzi

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you seeing a viable solution within these? Should we log this as debt and revisit?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't have a viable solution to suggest.
We need Keating & JP's input on that.

@scottrepreneur scottrepreneur merged commit 60a7fa2 into staging Aug 30, 2022
@scottrepreneur scottrepreneur deleted the fix/polyfill-buffer branch August 30, 2022 16:52
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.

None yet

4 participants