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

import walletconnect dependencies into app #420

Merged
merged 48 commits into from
Jun 27, 2023

Conversation

kremalicious
Copy link
Contributor

@kremalicious kremalicious commented Dec 1, 2022

It's as simple as that, or it should be.

Adding as PR as couldn't figure out how to get local version running, so hoping for PR preview deployments to test.

If indeed app is not setup to import either commonjs or esm packages, then most likely for this case here this rollup plugin needs to be setup:
https://github.com/rollup/plugins/tree/master/packages/commonjs

@vercel
Copy link

vercel bot commented Dec 1, 2022

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

Name Status Preview Comments Updated (UTC)
df-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 22, 2023 0:10am

public/index.html Outdated Show resolved Hide resolved
@idiom-bytes
Copy link
Member

Hi @kremalicious I'm reviewing this PR:

  1. The website starts breaking => https://df-9sf551740-oceanprotocol.vercel.app/
    I believe this is the result of the problem described here: index.js:419 Uncaught ReferenceError: require$$0$1 is not defined when I add @walletconnect/web3-provider sveltejs/svelte#6404

When @KatunaNorbert implemented WC, there were a handful of modifications needed in order to get it working.
7a4abc2

Perhaps there is something else that was overlooked?

As far as I can tell, this modification requires an import that may be throwing an error due to how the underlying .js code gets compiled.

@kremalicious
Copy link
Contributor Author

Most likely:

If indeed app is not setup to import either commonjs or esm packages, then most likely for this case here this rollup plugin needs to be setup:
https://github.com/rollup/plugins/tree/master/packages/commonjs

@KatunaNorbert
Copy link
Member

I already tried to add commonjs to rollup plugins but not working, that's why I didn't use the walletconnect dependency.
Will try to look more into enabling support for commonjs packages in this PR.

@idiom-bytes idiom-bytes marked this pull request as draft December 7, 2022 06:45
@idiom-bytes
Copy link
Member

Converted back to draft until there is a potential solution.

@kremalicious
Copy link
Contributor Author

ok, you might want to explore migrating to v2 instead of keeping this here as a draft until walletconnect v1 stops working completely. Basic problem and (react-only) options outlined here:
oceanprotocol/market#1828

@KatunaNorbert
Copy link
Member

KatunaNorbert commented Dec 16, 2022

I have been looking into migrating to WalletConnect v2 but, I run into the same import issue as before when trying to import WalletConnectProvider from @walletconnect/web3-provider. Now the issue appears when importing from @web3modal/ethereum.
They don't have support for react but they have for Vanilla Js so I was using that.

I've been also looking for other solutions for replacing WalletConnect but didn't find one.

@kremalicious
Copy link
Contributor Author

kremalicious commented Dec 19, 2022

Now the issue appears when importing from @web3modal/ethereum.

So this has to be solved, not finding another library which might use different exports. It's a generic problem that this app's build system is only setup for one specific type of javascript files. Every frontend framework like Next.js usually has this all setup under the hood, where it doesn't matter if the imported js is ES5, ES6, ESM, Node.js build, browser build, or whatever.

So the only reliable solution is to make this app's build system understand all the different types of imports one can encounter in the js ecosystem. Also switching/downgrading libraries to achieve that is not a reliable solution as those dependency authors might update their dependencies to use another export in future too.

As this project here opted to run and maintain its own custom build configuration via rollup, this needs to be solved there. This is a rollup problem, not necessarily to do with Svelte. I would suggest looking into frameworks on top of Svelte which have an existing build setup like Next.js has in background, and then look how they have done their rollup config and copy that.

Update:

What's obvious after quickly looking around for Svelte frameworks (like SvelteKit) is that they all do not use Rollup, but rather Vite or Webpack as build system, so kicking out Rollup and using one of those is another possibility. Which makes sense as Rollup is known to be extremely slow as a build tool.

If you want to continue to use Rollup, you should go through their docs thoroughly as they mention all their restrictions, some relevant snippets:

Unlike other bundlers such as Webpack and Browserify, Rollup doesn't know "out of the box" how to handle these dependencies - we need to add some configuration.

Note that most of the time @rollup/plugin-commonjs should go before other plugins that transform your modules

https://rollupjs.org/guide/en/#with-npm-packages

When building this branch also getting lots of errors in console about using native Node.js modules somewhere in code, but this is an app run in the browser in the end so native Node.js modules need to be polyfilled or even better never use native Node.js modules like fs, utils, require etc. in an app you intent to be ship and run in a browser.

But honestly, there have to be very good reasons for managing every small part of your build process in detail and I doubt doing this in here was a conscious decision based on a need to code your own build process. So the best path forward, but requiring most refactoring, would be to migrate to SvelteKit doing this for you. Like, SvelteKit reduces all that config you are doing with Rollup to this:

// vite.config.js
import { sveltekit } from '@sveltejs/kit/vite';

const config = {
  plugins: [sveltekit()]
};

export default config;
// package.json
"scripts": {
  "dev": "vite dev",
  "build": "vite build",
  "preview": "vite preview"
},

Done. With crazy fast development server. And you can import whatever you want out of the box. But would require some substantial refactoring, kicking out all rollup stuff and then moving things around according to required SvelteKit project structure.

So to summarize, the options to get this codebase into a state where ultimately the walletconnect packages can be simply imported would be, in order of low to high amount of refactor:

  1. Add things to Rollup config so es5, es6, commonjs, esm, etc. all can be imported.
  2. Switch to Vite or Webpack as build system and get a config working, then kick out Rollup.
  3. Switch to SvelteKit which handles most of the build config in the background with almost zero config.

@KatunaNorbert
Copy link
Member

@idiom-bytes Vercel Preview deployment it's now working and WC V2 it's ready to be tested

Copy link
Member

@idiom-bytes idiom-bytes left a comment

Choose a reason for hiding this comment

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

Reviewed code and tested off local. Tested lock, delegation, and farming. Did not test claim functionality. Looks great!

@idiom-bytes
Copy link
Member

Merge the PR, then delete Counter.svelte. 🙌

@KatunaNorbert KatunaNorbert merged commit 06a86e2 into main Jun 27, 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.

Replace WalletConnect from unpkg (June 2023)
4 participants