multiversx / mx-template-dapp-nextjs

https://template-dapp-nextjs.multiversx.com
7 stars 7 forks source link

DeFi wallet login buton breaks the Next.js build #52

Closed vladimirrostok closed 5 months ago

vladimirrostok commented 6 months ago

I always get this

ReferenceError: navigator is not defined
    at y (**\.next\server\chunks\2335.js:23:102029)
    at nM (**\node_modules\next\dist\compiled\next-server\app-page.runtime.prod.js:12:47419)
    at nN (**\node_modules\next\dist\compiled\next-server\app-page.runtime.prod.js:12:64674)
    at nI (**\node_modules\next\dist\compiled\next-server\app-page.runtime.prod.js:12:46806)
    at nM (**\node_modules\next\dist\compiled\next-server\app-page.runtime.prod.js:12:47570)
    at nN (**\node_modules\next\dist\compiled\next-server\app-page.runtime.prod.js:12:64674)
    at nB (**\node_modules\next\dist\compiled\next-server\app-page.runtime.prod.js:12:67657)
    at nF (**\node_modules\next\dist\compiled\next-server\app-page.runtime.prod.js:12:66824)
    at nN (**\node_modules\next\dist\compiled\next-server\app-page.runtime.prod.js:12:64990)
    at nB (**\node_modules\next\dist\compiled\next-server\app-page.runtime.prod.js:12:67657)

when I uncomment the following component at the /unlock page

              <ExtensionLoginButton
                onLoginRedirect={onLoginRedirect}
                loginButtonText="DeFi Wallet"
                {...commonProps}
              />

Other components work as expected, probably this is related to me missing some of wrappers at the code or this should work as expected if all other options work fine?

              <WalletConnectLoginButton
                loginButtonText="xPortal App"
                onLoginRedirect={onLoginRedirect}
                {...commonProps}
              />
              <LedgerLoginButton
                loginButtonText="Ledger"
                onLoginRedirect={onLoginRedirect}
                {...commonProps}
              />
               Comment this out as this breaks the build due to errors inside
              <ExtensionLoginButton
                onLoginRedirect={onLoginRedirect}
                loginButtonText="DeFi Wallet"
                {...commonProps}
              />
              <OperaWalletLoginButton
                loginButtonText="Opera Crypto Wallet - Beta"
                {...commonProps}
              />
              <WebWalletLoginButton
                loginButtonText="Web Wallet"
                data-testid="webWalletLoginBtn"
                {...commonProps}
              />
CiprianDraghici commented 5 months ago

Hey, any updates on this ?

vladimirrostok commented 5 months ago

ping here

CiprianDraghici commented 5 months ago

Hello everybody,

Here is the right place to report specific repository issues. For sure, there are many channels where the devs can ask for help, but reporting the issues directly on GitHub will help a lot also for tracking. We are trying to promptly address and prioritize each reported issue. However, we've noticed that some reports lack the necessary details, and others pertain to configuration or environment setup issues rather than problems with the repository itself. If there's been no activity following a report, it's likely that it's been assigned a lower priority or is awaiting additional information.

I fully support the idea of requesting clarification or more details before assigning priority to issues. I will also call for the implementation of specific requirements for issue reports to ensure clarity from the outset.

CiprianDraghici commented 5 months ago

@vladimirrostok, Creating a concise repository showcasing the issue would greatly aid in our troubleshooting efforts. The provided demo illustrates the expected functionality. Thus, preparing a succinct demo code facilitates quicker responses and solutions from the team.

vladimirrostok commented 5 months ago

Hey @CiprianDraghici, thanks for getting back to the thread to check if I still experience the issue!

Sorry for my misunderstanding above, I thought there was another person with a similar problem who requested an update on this so I double-pinged the issue. I was getting this error only when attempted to run the next build command, in dev mode everything was fine.

The build works on my local setup in a project now and most likely it was me using the older version of Node before, on v21.10.0 it works fine! I updated my Node version after I hit this issue, documenting the minimum required version would be nice.

The confusion originally came from me watching the last-merged PR and I noticed that the build here has failed too and the code was merged into the main https://github.com/multiversx/mx-template-dapp-nextjs/pull/50 image image

I didn't inspect the actual build problem back then, the issue was that the Vercel was set to Node v16, so the last build #50 failed when build #49 used Node v18 and it built fine so I thought there was a problem with the code in last pull-request #50.

To sum up, there is no code issue in the repo and the actual problem was my misunderstanding here.

Thanks to @MvXunity for clarifying the support channel!

vladimirrostok commented 5 months ago

@CiprianDraghici I can confirm that the build failed for me on DigitalOcean and Vercel after I uncommented this DeFi login option and pushed the project, I have no issues building the app only on my local setup

DigitalOcean project image Vercel project on both Node v18.X and v20.x returned me error too image image

Do you have any problem deploying the last version to Vercel? It would be great If you could re-run the deployment for the last #50 pull request after specifying Node v18-20 in Vercel because it clearly failed with v16 and we didn't see any successful build there, once we see it's building nicely that would clear the whole situation and I'll look for problems within my setup.

vladimirrostok commented 5 months ago

It looks like the latest Node 18 (18.19.1) throws this navigator error during the build, if I switch my local to Node v21 (21.1.0) it builds everything fine

Vercel is limited to v18 and v20 is still in beta there, that's why my app can't be built there (v20 failed too)

vladimirrostok commented 5 months ago

Node v21 actually has update to navigator object making it global, I see people struggle with this change https://github.com/nodejs/node/issues/50269 and https://github.com/framer/motion/issues/2376 for example, so there was some change that affected the navigator and this is why the Node v18 and v20 can't build project while it worked for me with my local v21.

Vercel does currently support only v18 and v20 https://vercel.com/docs/functions/runtimes/node-js#node.js-version and there's no option to hardcode v21 for now

If possible, please make sure this project in this repository builds with the current code in Vercel, re-running the build from #50 or making a new PR just to trigger the build is the best option as I'll be able to see the public Vercel check report from this PR too. @CiprianDraghici

CiprianDraghici commented 5 months ago

@vladimirrostok There's a pull request pending merging, which aims to verify that everything operates as intended. The PR will undergo several adjustments and enhancements. However, I cannot provide an estimated time of completion as we are awaiting releases for the 'sdk-core' and 'sdk-dapp' packages.

CiprianDraghici commented 5 months ago

@vladimirrostok please check this video

https://github.com/multiversx/mx-template-dapp-nextjs/assets/11082863/c85163c9-ed3e-48de-a9fe-c4aa994b819d

vladimirrostok commented 5 months ago

@CiprianDraghici Oh, thank you very much, this video is exactly what I needed to see!

Yeah, I was really really confused here as my build is sensitive to the Node version for some reason and this problem only occurs when I uncomment the DeFi option in my app and then it fails both on local and cloud setups including Vercel.

I will check everything in my application including the versions used in package .lock file, I will post the solution here once I fix this! There's a chance I've got some weird conflict somewhere in my app so I will clean it all up, and will also check if the standalone build output configuration specified in my project has any effect on this.

vladimirrostok commented 5 months ago

Hey! I found the exact problem and fix for this

The issue is here: https://github.com/multiversx/mx-sdk-dapp/blob/main/src/UI/extension/ExtensionLoginButton/ExtensionLoginButton.tsx#L50 image DeFi extension button uses a navigator function under the hood, if the following saying is correct „use client“ components run partly on the server, render what they can render, and hydrate at the client with the rest. Here you can find a good explanation: https://nextjs-faq.com/browser-api-client-component this is exactly what was happening in my project and breaking the build.

I don't know why this was triggered in my app and was ignored in demo recording, but when I applied this workaround it always works fine for me both on local and cloud setups (Vercel + DigitalOcean project)

I wrapped this button code in the unlock.tsx page

        <ExtensionLoginButton
          onLoginRedirect={onLoginRedirect}
          loginButtonText="DeFi Wallet"
          {...commonProps}
        />

with window condition check to prevent it from running on the server side

      {typeof window !== 'undefined' && (
        <ExtensionLoginButton
          onLoginRedirect={onLoginRedirect}
          loginButtonText="DeFi Wallet"
          {...commonProps}
        />
      )}

I think this should be prevented somewhere right in the library.

Fix works for me and my apps deployed to the cloud nicely, thanks for your help @CiprianDraghici !

CiprianDraghici commented 5 months ago

@vladimirrostok

The discovery you've made highlights an issue that is slated for resolution in an upcoming stable release of the sdk-dapp package. Your investigation is greatly appreciated! Since this issue doesn't impede progress, and the build operates smoothly with node version >= 18 using the "use client" directive, I'll go ahead and close it for now. We'll revisit it as updates become available.

image image

https://github.com/multiversx/mx-template-dapp-nextjs/assets/11082863/d8281913-c284-427e-8146-ce3f693a3449