Skip to content

Conversation

@PhearZero
Copy link

@PhearZero PhearZero commented Apr 6, 2025

Overview

The sdk looks great ❤️! I have some minor improvements/suggestions to help reduce downstream effects. (Sorry for the Info Dump, having coffee ☕ and diving in )

🤔 Proposal

Start to separate concerns between the stateful clients and allow composition in the "kitchen sink" package (nfd-sdk). This can be done by creating several modules which are consumed by the nfd-sdk module.

One of the examples that aligns best with the practice and the NfdClient would be Octockit. They have a rest namespace which is loaded by the kitchen sink module (octokit).

💡 Solution

A basic refactor which now pulls the generated fetch library from an independent package.

📝 Notes on implementation

✅ Package Provenance (ci changes)

Github/NPM now have provenance in their pipelines, I've updated the CI as an example.

📦 The great bundle debate!

I have a preference towards less obfuscation in the artifacts if it is at all possible. jsx-runtime or other difficult integrations make bundling a requirement but in pure libraries it's much less of a requirement. The main reason I argue for de-obfuscating the bundle is that it reveals hidden issues while also improving the tree-shaking capabilities of the library.

I've added my current meta to the nfd-fetch package. It treats every file as a bundle and leaves the artifacts as close to the original as possible. This brings to light non-trivial bundling issues, a good example is the externals for all export paths:

{
 rollupOptions: {
      external: [
        'algosdk',
        '@algorandfoundation/algokit-utils',
         // Needs several other paths to externalize fully
         '@algorandfoundation/algokit-utils/types/account',
        ...
      ],
  }
}

This is not easy to find when it is a single index or bundle of resources. If we leave the bundle de-obfuscated we can not only test each import in isolation for bundle quality, we can also allow downstream bundlers to better optimize the paths. With regards to CJS, Node 18 is EOL this month and the package is pinned to ^20, It should be safe to produce pure ESM at this point in time

🖋️ Conclusion

Hope this finds you well 🙏! Love all the work so far ❤️!! I think having the boundary early on will help the library be more flexible, API changes will not directly mean breaking changes since you can always augment the api-client. This should allow you to have power users adopt the other modules earlier while the majority of consumers are pinned to the sdk.

@PhearZero PhearZero force-pushed the refactor/fetch-library branch from b1e5e2e to 387a4d0 Compare April 6, 2025 15:07
@drichar
Copy link
Contributor

drichar commented Jul 8, 2025

@PhearZero I have not forgotten about this PR! I've been forced to aggressively prioritize other items and I'm sorry to keep you waiting so long.

I really like how splitting the fetch library into its own package separates concerns... not to mention the bundling improvements. So ready to drop CJS!

I will give this attention soon and will probably ship it as-is.

@PhearZero
Copy link
Author

Awesome @drichar no worries at all. I have been in a similar context and look forward to catching up!

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