Page MenuHomePhabricator

[Cashtab] Remove Prettier from dependency tree and align formatting in Web folder
ClosedPublic

Authored by emack on Oct 15 2021, 00:03.

Details

Summary

T1904

Workflow used

  • npm uninstall prettier [verify it's removed from package.json]
  • npm i -g prettier [installs the latest version]
  • cd web/
  • prettier write .
Test Plan
  • This patch only consists of code formatting changes.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
prettier-upgrade
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17070
Build 33973: Build Diffcashtab-components-tests · cashtab-tests
Build 33972: arc lint + arc unit

Event Timeline

emack requested review of this revision.Oct 15 2021, 00:03
deadalnix added inline comments.
web/cashtab/package.json
177 ↗(On Diff #30566)

I'm not sure why it's in here in the first place, arc isn't going to pick up this version of prettier so it seems rather useless.

Please run prettier --write . in the web/ folder to see if any changes come up

If none come up, then just abandon this diff. We'll bump the prettier version later on in a diff that upgrades the dependency tree.

If earlier whitespace removal was from IDE -- it's fine to do a diff that removes all the extra whitespace from cashtab, but then the diff should be focused on that.

This revision now requires changes to proceed.Oct 15 2021, 01:45

Executed prettier --write . in the web/ folder and here are the files modified in place

deadalnix requested changes to this revision.Oct 15 2021, 20:47

I'm sorry none of this makes sense.

If you run prettier int he web folder, without the original package change, you get the same result, right? Because you system's prettier is upgraded to 2.4.1, and this has nothing to do with the original change from this patch.

It's fine to upgrade prettier, but this whole thing just doesn't make sense. I don't know how or why prettier ended up in that package.json and if it is even useful to have it there, but when one stumble on bullshit of the kind, the right path forward to get it sorted out, not to add a second layer of shit on top it.

This revision now requires changes to proceed.Oct 15 2021, 20:47
bytesofman requested changes to this revision.EditedOct 15 2021, 21:18

To your point @deadalnix yeah, it doesn't make sense to have prettier in the actual repo, this is an older oversight. I directed @emack to upgrade it without considering this.

Intent of this diff is to just get all of the new formatting rules of the latest version of prettier applied across the whole web/ folder in one diff. That way, if new devs try to contribute to the repo and just have the latest version installed, we won't see occasional formatting differences come in that are not relevant to whatever diff they are working on.

So I think path fwd from here is

  • remove prettier from the dependency tree altogether
  • keep the above formatting changes

Removed Prettier from dependency tree and kept formatting changes

emack retitled this revision from [Cashtab] upgrade Prettier to latest version (2.4.1) to [Cashtab] Remove Prettier from dependency tree and align formatting in Web folder.Oct 16 2021, 01:22
emack edited the summary of this revision. (Show Details)
emack edited the test plan for this revision. (Show Details)

Checking this again -- the package-lock.json file should also be updated.

To remove prettier, run npm uninstall prettier from the web/cashtab folder

This revision now requires changes to proceed.Oct 17 2021, 10:33

Updated package-lock.json

To prevent users with older versions continuing to experience the same issue, add this line to the prettier object of the .arclint file in the top level of the repo

"version":">=2.4.1",
This revision now requires changes to proceed.Oct 18 2021, 20:55

Added minimum prettier versioning to .arclint in repo root

This revision is now accepted and ready to land.Oct 20 2021, 23:01