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

[1] feat!: stacksjs refactor #1622

Merged
merged 2 commits into from
Mar 7, 2024
Merged

[1] feat!: stacksjs refactor #1622

merged 2 commits into from
Mar 7, 2024

Conversation

janniks
Copy link
Collaborator

@janniks janniks commented Feb 5, 2024

EPIC 🏰 Breaking Refactoring Stacks.js

Goal: Make network/networking more obvious and less confusing for developers. While the default code will stay largely the same for most users, it should become more clear what node URL, and fetch implementation is used when.

291331642-81426bac-a6b0-4273-8c37-f92f4c8d8db6

  • Move to a network OBJECT approach (similar to bitcoin-js, micro-btc-signer, and others). The network will not be an instantiated class, but a static exported object with all information that might be relevant (txVersion, magicBytes, etc.)
  • Remove fetch functions from network and introduce StacksNodeApi, which can derive it's URL from a network, but should be treated separately -- more closely mirroring to what's actually happening in the background. Can be seen as a maintained API client, with post-processing of responses.
  • The majority of users doesn't realize network is also setting changes for "networking"
    • Most users don't customize fetchFn or URL. For users hosting their own node the only change becomes { api: { url: "my-node.com" }, ...} in the params of most relevant tx functions.

Supersedes #1596

@ Reviewers:
Please feel free to leave any feedback on these PRs. Going over the code will show what it's like to code in the new style. So any ideas and feedback (especially negative) is welcome 🙏


This PR:

  • removes duplicate definitions for next steps
  • refactors common package

Split into multiple PRs for easier reviewing

next
    ↑
feat/next-cleanup-common-files this PR 🟢
    ↑
feat/next-add-new-network #1623
    ↑
feat/next-add-api-package #1624
    ↑
feat/next-update-api-stacking #1625
    ↑
feat/next-update-cli #1626

Copy link

vercel bot commented Feb 5, 2024

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

Name Status Preview Comments Updated (UTC)
stacksjs-docs ❌ Failed (Inspect) Feb 5, 2024 3:40pm

@janniks janniks changed the title feat!: stacksjs refactor [1] feat!: stacksjs refactor Feb 5, 2024
This was referenced Feb 5, 2024
@hugocaillard
Copy link
Contributor

@janniks
So this PR is a small set of changes and is mostly just refactoring with almost no impact for developers right?

@janniks
Copy link
Collaborator Author

janniks commented Feb 12, 2024

Yes mostly small refactoring, however there are breaking changes with many consequences in the chain of PRs. I tried to split them up for easier review and following the refactor. Feel free to take a look at the other PRs mentioned in the description, some will be more breaking than others, but follow the same narrative.

Copy link
Contributor

@hugocaillard hugocaillard left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@janniks janniks merged commit a736172 into next Mar 7, 2024
7 of 9 checks passed
@janniks janniks deleted the feat/next-cleanup-common-files branch March 7, 2024 15:27
@janniks janniks restored the feat/next-cleanup-common-files branch March 12, 2024 01:36
@janniks janniks deleted the feat/next-cleanup-common-files branch March 12, 2024 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants