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

Feat/browser #54

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Feat/browser #54

wants to merge 15 commits into from

Conversation

anshss
Copy link
Member

@anshss anshss commented Jan 27, 2025

Making SDK workable in both browser and cli environments

Copy link

vercel bot commented Jan 27, 2025

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

Name Status Preview Comments Updated (UTC)
agent-wallet ❌ Failed (Inspect) Feb 6, 2025 8:59am

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this change would keep the aw-tool-registry from having the tool IPFS CIDs when building the monorepo locally if isNode isn't true. If so, how were you getting the tool IPFS CIDs?

Copy link
Member Author

Choose a reason for hiding this comment

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

this line says if the environment is node, user will be able to build and deploy tools using i.e using. the command pnpm deploy:tools

hitting this command makes list of tools and all tool-registry features available in browser app built over this sdk

Copy link
Member Author

Choose a reason for hiding this comment

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

lmk if this question is about something else

Copy link
Contributor

Choose a reason for hiding this comment

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

Without these changes were you having trouble using the packages within a browser env?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they were needed to make the SDK build-compatible with browsers, this is because CommonJS modules cannot directly run in browsers. They are designed for Node.js like environments, converting the exports will allow to ship a dual package that supports both ESM and CommonJS

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the this.log from this file. These will show up in user applications such as the CLI with no way for the user to silence these

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

2 participants