[ecashaddrjs] improvements for first abc release
AbandonedPublic
Actions

Authored by bytesofman on Mar 25 2023, 23:40.

Details

Reviewers

emack
Fabien

Group Reviewers

Restricted Project

Summary

T3039

Improvements to ecashaddrjs library in order to start publishing the npm module from bitcoin abc. No breaking changes.

Updated license, renamed dist files and main files, added support for prefixless addresses, bumped version

The dist/ files need to be included to support users who want minified / webpack versions for the browser.

Test Plan

Review changes and new unit tests
npm test

Diff Detail

Repository

rABC Bitcoin ABC

Branch

ecashaddrjs-updates

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 22707
Build 45034: Build Diff
Build 45033: arc lint + arc unit

Event Timeline

bytesofman created this revision.Mar 25 2023, 23:40

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 25 2023, 23:40

bytesofman requested review of this revision.Mar 25 2023, 23:40

Harbormaster completed remote builds in B22707: Diff 38901.Mar 25 2023, 23:41

emack accepted this revision.Mar 26 2023, 03:15

This revision is now accepted and ready to land.Mar 26 2023, 03:15

When your summary is a list of unrelated changes, it's a clear indication that this should be several diffs.

web/ecashaddrjs/.gitignore
7	Why ?
web/ecashaddrjs/LICENSE
5	?
web/ecashaddrjs/README.md
19	You should rather remove the version number form the readme, this is a maintenance nightmare and brings zero value. Also https://unpkg.com/ecashaddrjs points to the latest version anyway

This revision now requires changes to proceed.Mar 26 2023, 20:04

In D13469#299061, @Fabien wrote:

When your summary is a list of unrelated changes, it's a clear indication that this should be several diffs.

Yes, this could be split into a stack of say

Change all unit test addresses to ecash prefixes
Add support for prefixless addresses to decode
Update all licensing and github references
Deprecate webpack compilation in repo
Version bump to 1.1.0 + publish

Let me know if you think that's an improvement and I'll abandon and re-push in that format. From our discussion re: D13386, I got the impression your preference was to organize these improvements in one diff.

web/ecashaddrjs/.gitignore
7	The issue is that a module written in nodejs (like this one) may not work in the browser unless it has been compiled to specifically work in the browser. Webpack does this (i.e. running "npm run build" in this module). For web developers, this package may not work "as is" -- it depends on how their app is structured. They may need to use the minified version. In Cashtab it "just works" because Cashtab uses webpack to convert all of the code into the production app, which includes the installed dependencies. For some web apps, though, they may want a dependency that has already been minified with webpack. That said...it is probably okay to deprecate this support. Web development has come a long way and it is pretty standard now for every app to handle its own js compilation for browser support. If we we deprecate this support though we should do it in a specific diff / version bump, and probably bump the minor version number when we do this.
web/ecashaddrjs/LICENSE
5	I added the license found in the monorepo: https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/COPYING
web/ecashaddrjs/README.md
19	this version (1.0.8) is not yet published to npm, so the link does not work, however you can see that https://unpkg.com/ecashaddrjs@1.0.7/dist/cashaddrjs-1.0.7.min.js is different from https://unpkg.com/ecashaddrjs the `/dist/` version has been combiled to work in the browser. From above comment -- we probably can and should deprecate this support, but I think we should do that as a standalone diff and bump the minor version number when we do this. Also yeah...changing the name is a total maintenance nightmare, but it is standard practice for `npm` modules that older versions are available. I think the solution is to deprecate support for pre-compiled browser support minified js files in a later diff.

thinking about it, we don't need to keep supporting the minified. users who need that can just use 1.0.7

will repush this as the stack described above

From our discussion re: D13386, I got the impression your preference was to organize these improvements in one diff.

My preference is to organize the diffs so they can be:

Reviewed
Properly tested
Easily reverted

Reviewed:
You want to review the code that is added/changed in the diff, and nothing else. The diffs must be self-contained.
Each diff must bring value on its own. If the value is not clear from the code, the summary should clarify why the change is needed.

The diff D13386 was about adding the lib to the monorepo. So I reviewed the code being added and found several files/features that were not needed, as well as some nits like the license being inconsistent or the missing prefix detection feature.
Because the object of the diff is to add to code to the repo, adding unnecessary files and features are a blocker because they will remain forever under source control for no reason, while the nits are not as they don't break anything and can be fixed in follow-ups like this one.

If you look at this diff: it does several things that have value on their own but are blocked because of the other changes. I didn't look at the details but the files renaming seems to be OK for example, but now it is blocked and not landed yet because there is a discussion regarding the release process, which is totally unrelated. As long as you're working alone on the code that might be manageable, but if Ethan wants to work on that library now you will get rebase conflicts because of that, and a higher risk of breaking something due to conflict resolution.

Properly tested:
This is correctly done in this diff. You added support for prefixless addresses and this comes with tests. The test plan also tells how to run the tests.
The only missing part is the CI: here I have to trust that you ran the tests. CI integration would remove this assumption, but this is unrelated to the added feature and must be its own diff.

Easily reverted:
It's likely the most under-evaluated reason for splitting the changes into self-contained diffs. Let's imagine the prefixless support change breaks the library and makes it unusable: you might need to revert the changes as a hotfix.
This would lead to the reversal of all the other change as well, even if they are all correct and unrelated to the bug you're trying to fix. Even worst it may even make it impossible to revert the commit at all, depending on what you squashed into the diff, like a build fix for example.

I hope this clarifies the methodology a bit and I would like to make sure that this is applied not only during the diff creation but also at review time.

web/ecashaddrjs/.gitignore
7	There is a misunderstanding here. I don't mind if we ship a version located in a dist/ folder via unpkg.org, minified or not. But the output of the build doesn't belong to the source tree, and should never be. We can automatically generate it and push it upon version change for example.
web/ecashaddrjs/LICENSE
5	AFAIK core never contributed to cashaddrjs, and this is not a fork of a core repo so it's not needed. Also we have no code from 2009 in this project. We should probably have another file at the root of the repo to explain that there are several projects and all have a different (but compatible) license.
web/ecashaddrjs/README.md
19	Can we just simply publish https://unpkg.com/ecashaddrjs@1.0.8/dist/cashaddrjs.min.js (note the version is removed from the file name) ? So we still have versioning support but no more messing with file names.

Got it. This is has been repushed as a series of smaller diffs.

On consideration, opted not to change the filenames or module names from cashaddr to ecashaddr. Although this change would not require users to change their implementations (i.e. the module could still be imported with const cashaddr = require('ecashaddrjs');, it could lead to some confusion. In general, a user wanting to use this repo for ecash support will find out very quickly they don't have the right thing installed if they are using cashaddrjs, as this repo does not regard ecash: as a valid prefix.

D13479

At this point, alias-server, ecash-telegram-bot, and ecashaddrjs all use nyc mocha for unit tests. So, CI implementation should be handled separately in a diff that does this for all node apps.