Skip to content
This repository was archived by the owner on Mar 26, 2025. It is now read-only.

Makes BundlrStorageDriver work in browser contexts without polyfills#379

Merged
lorisleiva merged 3 commits intometaplex-foundation:mainfrom
thirdweb-dev:main
Nov 9, 2022
Merged

Makes BundlrStorageDriver work in browser contexts without polyfills#379
lorisleiva merged 3 commits intometaplex-foundation:mainfrom
thirdweb-dev:main

Conversation

@jnsdls
Copy link
Copy Markdown
Contributor

@jnsdls jnsdls commented Oct 27, 2022

We've been getting reports from our users that installing @thirdweb-dev/sdk requires them to polyfill CRA and vite applications.

I've hunted down the issue to be within @metaplex-foundation/js -> @bundlr-network/client.

After some testing I found the simplest fix to be importing the @bundlr-network/client dependency with a dynamic import() instead of top level, which means the resulting bundle will load the correct version at runtime for web vs node contexts.

Please let me know if there's anything I'm missing here, it seems to have no negative side-effects in my testing.

@lorisleiva
Copy link
Copy Markdown
Contributor

Hi there 👋

Thanks for this. I'm thinking of extracting this Bundlr driver as an external plugin soon so people don't inherit its dependencies by default anymore.

I'm happy to ship this fix in the meantime but it looks like the tests aren't passing. Would you mind having a look at what's going on?

@jnsdls
Copy link
Copy Markdown
Contributor Author

jnsdls commented Oct 31, 2022

Thanks for this. I'm thinking of extracting this Bundlr driver as an external plugin soon so people don't inherit its dependencies by default anymore.

This would be an even better way to fix the issue, in our case we don't actually use bundlr for anything (as of now) so that would solve it for us. (But prob still want to have even that external plugin work in both browser and node.)

I'm happy to ship this fix in the meantime but it looks like the tests aren't passing. Would you mind having a look at what's going on?

Yep, I'm back in the office tomorrow so will take a look then and update here if/when I find what's going on.

@jnsdls
Copy link
Copy Markdown
Contributor Author

jnsdls commented Nov 1, 2022

hey @lorisleiva, been trying to hunt down the test failure but I'm running into some issues:

  • tried running tests locally, amman instance OOM error (node process runs out of memory)
  • tried running tests locally straight from main, still running out of memory (however I see this seems to work in GH actions on main so I have no idea why this would be the case

Have you ran into issues where the amman node process runs out of memory while running tests?

Edit: I'm running node 16 on m1 MBP for this fwiw.

@jnsdls
Copy link
Copy Markdown
Contributor Author

jnsdls commented Nov 1, 2022

Further investigation results:

  • cloned an entirely clean copy of main
  • ran tests -> all cached so no problems ✅
  • added a useless comment (// hello I will break tests) in BundlrStorageDriver.ts (just to break cache) -> build -> test -> amman OOM ❌

seems like the same is possibly happening in #380?

Let me know if you want a pure reproduction case. (Though all it takes is take main, add a comment in the file to break cache, try to build & test -> OOM)

@jnsdls
Copy link
Copy Markdown
Contributor Author

jnsdls commented Nov 1, 2022

so it seems to me like bundlr plugin is used at the very root level any change to it causes a full re-test of everything and that makes amman fall over because of some amman-internal (?) memory leak (?)

@lorisleiva
Copy link
Copy Markdown
Contributor

Hey @jnsdls This is an issue with the Amman relay which seems to stop working when too many tests are executed consecutively.

Can you run export CI=1 locally on your terminal session before running yarn amman:start and before running the tests? This basically disables the Amman relay on the terminal.

@jnsdls
Copy link
Copy Markdown
Contributor Author

jnsdls commented Nov 2, 2022

Hey @jnsdls This is an issue with the Amman relay which seems to stop working when too many tests are executed consecutively.

Can you run export CI=1 locally on your terminal session before running yarn amman:start and before running the tests? This basically disables the Amman relay on the terminal.

will try this, thanks!

EDIT: when running with export CI=1 the amman server just shuts down immediately after startup ¯\_(ツ)_/¯

@jnsdls
Copy link
Copy Markdown
Contributor Author

jnsdls commented Nov 3, 2022

@lorisleiva any hints on this would be appreciated.

I'm unable to run the tests with export CI=1 and have been unable to find a solution to that.

@lorisleiva
Copy link
Copy Markdown
Contributor

I've just had a look at your PR and the test that fails is the one that executes when you run yarn test:exports. These particular tests ensure that a library can be exported for both ES Modules and CommonJS. It looks like your changes are not ESM compatible as the following error gets thrown:

  [esm] it can import the Bundlr client


    ✖ TypeError: bPackage.WebBundlr is not a constructor
    -----------------------------------------------------
      operator: error
      stack: |-
  TypeError: bPackage.WebBundlr is not a constructor
  at BundlrStorageDriver.initWebBundlr (file:///Users/loris/Code/@metaplex/js/packages/js/dist/esm/plugins/bundlrStorage/BundlrStorageDriver.mjs:164:20)
  at processTicksAndRejections (node:internal/process/task_queues:96:5)
  at async BundlrStorageDriver.initBundlr (file:///Users/loris/Code/@metaplex/js/packages/js/dist/esm/plugins/bundlrStorage/BundlrStorageDriver.mjs:129:16)
  at async BundlrStorageDriver.bundlr (file:///Users/loris/Code/@metaplex/js/packages/js/dist/esm/plugins/bundlrStorage/BundlrStorageDriver.mjs:109:27)
  at async Test.<anonymous> (file:///Users/loris/Code/@metaplex/js/packages/js/test/esm-export.test.mjs:18:18)

Note that the same test passes in CommonJS.

@jnsdls
Copy link
Copy Markdown
Contributor Author

jnsdls commented Nov 4, 2022

Ok I should be able to run that test individually then which should mean I don't need to figure out how to get Amman to run on my local without running OOM / shutting down on startup. Thank you, I'll look into it and get it fixed today!

@jnsdls
Copy link
Copy Markdown
Contributor Author

jnsdls commented Nov 4, 2022

@lorisleiva could you run tests again on this, I believe it should work now!

@jnsdls
Copy link
Copy Markdown
Contributor Author

jnsdls commented Nov 7, 2022

yay, passes now, thanks for your help with this @lorisleiva!

@lorisleiva lorisleiva merged commit d8d97ba into metaplex-foundation:main Nov 9, 2022
@lorisleiva
Copy link
Copy Markdown
Contributor

Thank you! I'll add this to the next patch release asap.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants