Skip to content

refactor: fetch library #12

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

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

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
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.

1 participant