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

add required peer deps to the framegear package #187

Merged
merged 4 commits into from
Feb 24, 2024

Conversation

DonGambas
Copy link
Contributor

What changed? Why?

added below dependencies to the package.json in the framegear package. these are required peer dependencies by @coinbase/onchainkit

{
    "graphql": "^14.0.0",
    "graphql-request": "^6.0.0",
    "viem": "^2.7.0",
}

Notes to reviewers

when running framegear for the first time warnings are raised for the above 3 dependencies. can either add them here to the package.json or add note in documentation that they need to be installed before running framegear.

How has it been tested?

after adding these deps warnings are not raised when installing deps with yarn.

@DonGambas DonGambas changed the title add required peer deps add required peer deps to the framegear package Feb 24, 2024
@cnasc
Copy link
Contributor

cnasc commented Feb 24, 2024

Hi @DonGambas, if these are the warnings you're talking about:

➤ YN0002: │ framegear@workspace:. doesn't provide graphql (p3f427), requested by @coinbase/onchainkit.
➤ YN0002: │ framegear@workspace:. doesn't provide graphql-request (pe7063), requested by @coinbase/onchainkit.
➤ YN0002: │ framegear@workspace:. doesn't provide viem (pe026c), requested by @coinbase/onchainkit.

framegear doesn't use any code from onchainkit that requires them, which is why they aren't installed

@DonGambas
Copy link
Contributor Author

DonGambas commented Feb 24, 2024

Hey @cnasc, those are the warnings that are raised at point of package install (which doens't seem to be problematic), but when I try to call postFrame I get the below import error, looks like there are probably some downstream dep issues in onchainkit.

Module not found: Can't resolve 'viem/chains'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/@coinbase/onchainkit/lib/utils/easAttestation.js
./node_modules/@coinbase/onchainkit/lib/core/getEASAttestations.js
./node_modules/@coinbase/onchainkit/lib/index.js
./app/api/postFrame/route.ts

steps to reproduce:

  1. clone fresh copy of onchainkit
  2. run yarn install
  3. run yarn dev
  4. load a frame into the UI that includes a fc:frame:post_url property (can test with https://degen-frame.vercel.app/)
  5. click on button that posts to post_url

feel free to close this if not helpful since it doesn't appear to be a peer dep issue.

@Zizzamia
Copy link
Contributor

@DonGambas oh I see, what version of OnchainKit are you using? And how are you using OnchainKit.

The error you have is an old issue, I believe by upgrading to the latest version you should be fine.

Also just to triple check, can you please share how you import the code from OnchainKit.

@DonGambas
Copy link
Contributor Author

@Zizzamia I was just running the framegear package standalone after cloning the repo (didn't update any imports etc).

It does look like the framegear package was using an older version of onchain kit. I upgraded onchain kit, ran the steps to reproduce listed above, and ran into another viem import error (see below).

It looks like anything you import from onchainkit throws an error if viem isn't installed. I updated PR with latest version of onchainkit and viem and no issue. There's probably a longer term solution to this (linking parent package version to sibling package deps so they're always up to date), but for now these changes appear to resolve the issue, but I might be missing something.

Module not found: Can't resolve 'viem/ens'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/@coinbase/onchainkit/lib/index.js
./app/api/postFrame/route.ts

@Zizzamia
Copy link
Contributor

Zizzamia commented Feb 24, 2024

Thank you for this upgrade!!!

Oh, so I think we can get rid of Viem as well, if you import from getMockFrameRequest from @coinbase/onchainkit/frame.

So you do

import { getMockFrameRequest } from '@coinbase/onchainkit/frame';

That's good to know, I need to document getMockFrameRequest in https://onchainkit.xyz

Btw, thank you again for going after those bugs. Really appreciated.

@Zizzamia
Copy link
Contributor

Long story short, OnchainKit will have several dependencies depending of the feature you need. So to make imports more clean we started recently exploring with namespaces. BUT we don't want to break existing app, so we kept the original imports as they are.

Eventually when we will bump from v0 to v1, we will make a clean breaking changes.

@Zizzamia
Copy link
Contributor

Merging this, as it is already a good progress, and do a quick follow up on my own.

@Zizzamia Zizzamia merged commit 33db69b into coinbase:main Feb 24, 2024
5 of 6 checks passed
@DonGambas
Copy link
Contributor Author

awesome, thanks for the clear explanation of how this is evolving, moving towards using more namespaces makes complete sense.

@DonGambas DonGambas deleted the cm/add-deps branch February 24, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants