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

Runtianz/new script composer #587

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

Conversation

runtian-zhou
Copy link
Contributor

Description

Reopen the script composer feature with updated dependency built into aptos-labs.

Test Plan

CI

Tested that the package can work in web, nodejs and nextjs environment.

Related Links

#565

Checklist

  • Have you ran pnpm fmt?
  • Have you updated the CHANGELOG.md?

@0xmaayan
Copy link
Collaborator

Please test and validate it

  1. Passes all SDK unit + examples tests
  2. Does not break a client-side render app (react)
  3. Does not break a server-side render app (nextjs)

@WGB5445
Copy link
Contributor

WGB5445 commented Nov 20, 2024

Please test and validate it

  1. Passes all SDK unit + examples tests
  2. Does not break a client-side render app (react)
  3. Does not break a server-side render app (nextjs)

I have tested a React + Vite client-side render app and a Next.js server-side render app.
Both work fine.

However, it's worth noting that in the current version, the initialization is done when the SDK loads, so the bundled JavaScript is a bit large—about 5 MB for the client-side.

@0xmaayan
Copy link
Collaborator

0xmaayan commented Nov 21, 2024

Please test and validate it

  1. Passes all SDK unit + examples tests
  2. Does not break a client-side render app (react)
  3. Does not break a server-side render app (nextjs)

I have tested a React + Vite client-side render app and a Next.js server-side render app. Both work fine.

However, it's worth noting that in the current version, the initialization is done when the SDK loads, so the bundled JavaScript is a bit large—about 5 MB for the client-side.

The bundle size is a concern. I'd like to get consensus from the sdk team before approving it. @gregnazario @GhostWalker562

@GhostWalker562
Copy link
Contributor

GhostWalker562 commented Nov 21, 2024

@0xmaayan I personally think its too much 😞 since it's already 5x the bundle size. People are already complaining about 421kb from poseidon-lite #578 but if it's really needed in the SDK I'm not going to push back that much.

I'm curious if we can test it in a react-native environment as well?

@0xmaayan
Copy link
Collaborator

@0xmaayan I personally think its too much 😞 since it's already 5x the bundle size. People are already complaining about 421kb from poseidon-lite #578 but if it's really needed in the SDK I'm not going to push back that much.

I'm curious if we can test it in a react-native environment as well?

@runtian-zhou

  1. Any reason we dont move the wasm initialization to the end user?
  2. Thoughts on releasing it under a tagged version?

@GhostWalker562 I dont think the SDK officially supports react-native, but I assume you want it tested for Petra?

@runtian-zhou
Copy link
Contributor Author

Thoughts on releasing it under a tagged version?

I think that would be amazing.

@0xmaayan
Copy link
Collaborator

Thoughts on releasing it under a tagged version?

I think that would be amazing.

Cool. Then we dont need to merge it to main, but release a tagged version from this branch

@runtian-zhou
Copy link
Contributor Author

Let's release it first and get feedbacks from the community. I'm getting multiple asks for this feature so would be great to let them try it out first. In the meantime, we can optimize on the packaging situation.

@lumos42
Copy link

lumos42 commented Nov 21, 2024

eagerly waiting to use this feature in our app

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.

5 participants