refactor(ui): source browsing by sarahscott · Pull Request #995 · radicle-dev/radicle-upstream

sarahscott · 2020-10-06T00:37:02Z

Why?

When we created Remote.svelte, we didn't have the option to type-check data in components. To enable its usage as we migrate from JS -> TS, I asserted an any type here:

  export let store: Readable<remote.Data<any>>;

...which essentially destroys any notion of type safety in the data value (YOLO🤪). While this is convenient, it doesn't allow us to use the magic of the type system in the most critical places - i.e. when we're rendering data that we expect to be of a certain shape.

The Remote.svelte component does make it very easy to load slots based on the remote store status (Loading/Success/Error), which I think is ergonomic and useful and can perhaps be its only function until we have generics in svelte. With this in mind, I'd like to transition away from the let:data={something} pattern and towards one where we know exactly what the T in Data<T> is before we render components.

To kick off this effort, I've typed the Project screen and its associated components, created a single source of truth for the project -> peer -> revisions -> commits data flow, and added a lot of comments to clarify what exactly these types are supposed to be.

Closes #970

MeBrei

Great effort tackling the refactoring of this code! When working on it, I also felt the need for it, but didn't quite know where to start.

I am a bit worried, changing this code while we are not able to test it properly. Tests related to the peer selector are currently skipped and we also can't test that in the app directly.

I also found the following regression:

when switching from one project to another the page of the second project does not load and errors with the following:

cypress/integration/project_source_browsing.spec.js

ui/src/source.ts

MeBrei · 2020-10-06T09:31:12Z

It seems like your changes also solved #970

sarahscott · 2020-10-06T22:48:21Z

@MeBrei fixed the regression! I'd say it's the perfect time to strongly type everything peer-related so it's easier to reason about when we have actual peers to test 😁

cypress/integration/project_source_browsing.spec.js

MeBrei

Looks good to me! Just added a comment for understanding the new way of using remote.

I think the tests are failing, because some contains have to be replaced with pick and using data-cy. The workaround for wait only works with get / pick. (One example is trying to do cy.contains(".i-am-well-hidden").click(); in the source browsing specs.

MeBrei · 2020-10-07T06:26:00Z

ui/Screen/Project.svelte

-                  bind:currentRevision={$currentRevision}
-                  {revisions} />
+  <Remote {store} context="project">
+    {#if project}


Why do we need this check? I thought that the slot (without name) in Remote is only rendered on Success.

With the old patten (let:data={project}), that was the case, but now project is supplied by the store itself within the <script> tags, and its type is Project | undefined.

Also, Success doesn't necessarily mean there is data - it could be possible in some cases for endpoints to return empty arrays successfully (e.g. the projects/ endpoint could return [] if the user has no projects).

FintanH

Mostly observing 👀

ui/src/identity.ts

FintanH · 2020-10-07T08:35:53Z

ui/src/source.ts

  name: string;
-  peerId?: string;
+  // The id of the project this branch belongs to
+  projectId: string;


Suggested change

projectId: string;

projectUrn: string;

This is worthy of its own pr because Project has an id in proxy - relevant to #840

ui/src/source.ts

FintanH · 2020-10-07T08:40:28Z

ui/src/source.ts

+    const defaultRevision =
+      defaultRevisions.branches.find(
+        branch => branch.name === defaultRevisions.defaultBranch
+      ) || defaultRevisions.branches[0];


And in the event that there are no branches?

In this case, the revision selector won't show up but the project will still render. Before, the RevisionSelector would create a default revision that may or may not actually exist (https://github.com/radicle-dev/radicle-upstream/pull/995/files#diff-ac6e05ed01605cc8ea4ba88d402e6541L30).

Should we throw an error instead? I don't really like the idea of creating a revision in ui that we don't actually receive from proxy.

Ya, I definitely agree that creating a made-up revision doesn't make sense. Could we have like an error window in the selector saying something like, "We couldn't find any revisions o.O"?

Again, some sort of error handling would be good to think about when we see partial functions like these :)

Same question here about whether we'll address these here.

FintanH · 2020-10-07T08:51:39Z

ui/Screen/Project/PeerSelector.svelte

  };
+
+  $: currentPeer =
+    availablePeers.find(peer => peer.id === currentPeerId) || availablePeers[0];


Again, what happens if there are no availablePeers?

The PeerSelector wouldn't render in that case - this would mean that proxy didn't return any Revisions, or that it returned Revisions without an Identity, which I don't think it's supposed to do 🤔 .

Alternatively we could throw an error, or display the disabled PeerSelector with some sort of 'No peers found" messaging.

which I don't think it's supposed to do .

Don't assume external resources will always do the thing they're supposed to. That's what I'm trying to get at here: we should think about failure cases and show appropriate errors when possible. Indexing arrays is a code smell in my eyes.

@xla do you plan to do anything about this in this PR, out of interest?

xla

Dope! Currently we can't just export let store: Readable<remote.Data<T>>; the situation?

ui/DesignSystem/Component/HorizontalMenu.svelte

ui/DesignSystem/Component/HorizontalMenu/MenuItem.svelte

ui/DesignSystem/Component/SourceBrowser/RevisionSelector.svelte

ui/Screen/Project.svelte

ui/src/transaction.ts

sarahscott · 2020-10-07T23:56:39Z

Dope! Currently we can't just export let store: Readable<remote.Data>; the situation?

@xla we can't export generics from svelte components 😢
sveltejs/rfcs#38

…le-dev/radicle-upstream into sos/typing-source-browsing

all points were addressed

Adressed

xla · 2020-10-18T12:58:23Z

Took this on and got it up-to-speed. PTAL.

rudolfs · 2020-10-19T07:22:20Z

Steps to repro:

add radicle-upstream as a project
click on a file in the file browser, for example CHANGELOG.md
navigate back to your profile by clicking the profile icon in the sidebar
go back to the project by clicking on radicle-upstream in the project list

Expected result: see the project source.
Actual result: exception in the console.

FintanH · 2020-10-19T09:06:00Z

ui/Screen/Project/PeerSelector.svelte

  };
+
+  $: currentPeer =
+    availablePeers.find(peer => peer.id === currentPeerId) || availablePeers[0];


@xla do you plan to do anything about this in this PR, out of interest?

FintanH · 2020-10-19T09:08:18Z

ui/src/source.ts

 }

+// Currently supported revision types in UI:
+export type SupportedRevision = Branch | Tag;


Is Sha not supported? 🤔 Or what's the issue here?

FintanH · 2020-10-19T09:09:28Z

ui/src/source.ts

+    const defaultRevision =
+      defaultRevisions.branches.find(
+        branch => branch.name === defaultRevisions.defaultBranch
+      ) || defaultRevisions.branches[0];


Same question here about whether we'll address these here.

FintanH · 2020-10-19T09:10:22Z

ui/src/source.ts

-export const currentPeerId = writable(null);
-export const resetCurrentPeerId = (): void => currentPeerId.set(null);
+
+export const currentRevision = writable<Branch | Tag | undefined>(undefined);


Suggested change

export const currentRevision = writable<Branch | Tag | undefined>(undefined);

export const currentRevision = writable<SupportedRevision | undefined>(undefined);

We could re-use the above type right?

FintanH · 2020-10-19T09:10:40Z

ui/src/source.ts

  kind: Kind.FetchCommits;
  projectId: string;
-  revision: Branch;
+  revision: Branch | Tag;


Suggested change

revision: Branch | Tag;

revision: SupportedRevision;

FintanH · 2020-10-19T09:11:11Z

ui/src/source.ts

+
+interface UpdateCurrentRevision extends event.Event<Kind> {
+  kind: Kind.UpdateCurrentRevision;
+  revision: Branch | Tag;


Suggested change

revision: Branch | Tag;

revision: SupportedRevision;

It's not used and generates svelte-check warnings.

sarahscott added 6 commits October 1, 2020 15:38

starting the process

02c2321

revision & peer selectors

2c29219

move logic into project

717904f

cleanup

99f3914

fix source browsing tests

0f5ab2a

Merge branch 'master' into sos/typing-source-browsing

c0591be

sarahscott added improvement labels Oct 6, 2020

sarahscott requested review from FintanH, MeBrei, NunoAlexandre and xla October 6, 2020 00:37

sarahscott self-assigned this Oct 6, 2020

more comments

47f23ac

sarahscott force-pushed the sos/typing-source-browsing branch from 92ac06f to 47f23ac Compare October 6, 2020 00:51

MeBrei previously requested changes Oct 6, 2020

View reviewed changes

cypress/integration/project_source_browsing.spec.js Outdated Show resolved Hide resolved

ui/src/source.ts Show resolved Hide resolved

sarahscott added 2 commits October 6, 2020 07:52

fix peer selector

127b11d

actually fix tests

4638cb8

Merge branch 'master' into sos/typing-source-browsing

09af3dd

sarahscott commented Oct 6, 2020

View reviewed changes

cypress/integration/project_source_browsing.spec.js Outdated Show resolved Hide resolved

MeBrei reviewed Oct 7, 2020

View reviewed changes

FintanH reviewed Oct 7, 2020

View reviewed changes

xla previously requested changes Oct 7, 2020

View reviewed changes

Merge branch 'master' into sos/typing-source-browsing

43e02d6

sarahscott added 3 commits October 7, 2020 17:16

feeedback

2bc22ea

Merge branch 'master' into sos/typing-source-browsing

75ee113

Merge branch 'sos/typing-source-browsing' of https://github.com/radic…

9f6feb9

…le-dev/radicle-upstream into sos/typing-source-browsing

xla marked this pull request as draft October 14, 2020 12:38

xla assigned xla and unassigned sarahscott Oct 14, 2020

xla added 4 commits October 18, 2020 13:51

Merge branch 'master' into sos/typing-source-browsing

bba338f

Fix checkout

5130537

Fix exisitng project name validation

e6b6a7c

Add missing return types

dd7c9c3

xla requested review from FintanH, MeBrei, rudolfs and xla October 18, 2020 12:57

xla marked this pull request as ready for review October 18, 2020 12:57

FintanH reviewed Oct 19, 2020

View reviewed changes

rudolfs added 8 commits October 20, 2020 08:47

Merge branch 'master' into sos/typing-source-browsing

93ce2dd

Fix imports

f315aff

Merge branch 'master' into sos/typing-source-browsing

6940504

Remove Placeholder component

c4b7ab6

It's not used and generates svelte-check warnings.

Remove superfluous prop

d1f5857

Fix last svelte-check complaint

ae8d783

Fix exception when switching between projects

bdd6375

Merge branch 'master' into sos/typing-source-browsing

8891740

rudolfs mentioned this pull request Oct 20, 2020

Project readme flickers on branch switch #970

Closed

Merge branch 'master' into sos/typing-source-browsing

e62417f

xla closed this Oct 24, 2020

rudolfs deleted the sos/typing-source-browsing branch February 10, 2021 11:01

	export const currentRevision = writable<Branch \| Tag \| undefined>(undefined);
	export const currentRevision = writable<SupportedRevision \| undefined>(undefined);

Conversation

sarahscott commented Oct 6, 2020 • edited by xla Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MeBrei left a comment • edited by sarahscott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MeBrei commented Oct 6, 2020

Uh oh!

sarahscott commented Oct 6, 2020

Uh oh!

Uh oh!

MeBrei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahscott Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FintanH left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarahscott commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xla commented Oct 18, 2020

Uh oh!

rudolfs commented Oct 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

sarahscott commented Oct 6, 2020 •

edited by xla

Loading

MeBrei left a comment •

edited by sarahscott

Loading

sarahscott Oct 7, 2020 •

edited

Loading

sarahscott commented Oct 7, 2020 •

edited

Loading