[DRAFT] Workspace layer refactor #7657

darzu · 2020-11-23T21:02:56Z

While working on cloud sync, I ended up discovering a bunch of inconsistencies and potential race conditions and other problems in our workspace layering, so I've started a clean up and refactor of that area of the code.

This is going on the back-burner however and we're going to take a simpler, incremental approach to cloud sync for now here: #7743

EDIT: closed and reopened here: #7902

darzu · 2020-11-23T21:10:00Z

localtypings/projectheader.d.ts

@@ -28,7 +28,8 @@ declare namespace pxt.workspace {
        tutorialCompleted?: pxt.tutorial.TutorialCompletionInfo;
        // workspace guid of the extension under test
        extensionUnderTest?: string;
-        cloudSync?: boolean; // Mark a header for syncing with a cloud provider


note:
was used to determine if a project was cloud synced (old Sam work?)
problem was it didn't distinguish account A or B

localtypings/projectheader.d.ts

darzu · 2020-11-23T21:11:56Z

localtypings/projectheader.d.ts

@@ -28,7 +28,8 @@ declare namespace pxt.workspace {
        tutorialCompleted?: pxt.tutorial.TutorialCompletionInfo;
        // workspace guid of the extension under test
        extensionUnderTest?: string;
-        cloudSync?: boolean; // Mark a header for syncing with a cloud provider
+        // id of cloud user who created this project
+        cloudUserId?: string;
    }

    export interface Header extends InstallHeader {


header is meta info for a project

darzu · 2020-11-23T21:12:37Z

localtypings/projectheader.d.ts

+        blobCurrent_: boolean; // has the current version of the script been pushed to cloud
+
+        cloudVersion: string;     // The cloud-assigned version number (e.g. etag)
+        cloudCurrent: boolean;    // Has the current version of the project been pushed to cloud


TODO: do we need / how do we use cloudCurrent exactly?

darzu · 2020-11-23T21:14:13Z

localtypings/projectheader.d.ts

-        blobVersion: string;  // version of the cloud blob
-        blobCurrent: boolean; // has the current version of the script been pushed to cloud
+        // For cloud providers -- DEPRECATED
+        blobId_: string;       // id of the cloud blob holding this script


note that "project id" is assigned by client and its user ID + project ID. Note "id" field

darzu · 2020-11-23T21:14:43Z

pxteditor/editor.ts

@@ -215,12 +215,11 @@ namespace pxt.editor {
        importExampleAsync(options: ExampleImportOptions): Promise<void>;
        showScriptManager(): void;
        importProjectDialog(): void;
-        cloudSync(): boolean;
-        cloudSignInDialog(): void;


cloudSignInDialog is unneeded because we have new dialog

darzu · 2020-11-23T21:15:16Z

pxteditor/editor.ts

        removeProject(): void;
        editText(): void;

+        hasCloudSync(): boolean;


renamed from cloudSync to find usages

checks to see if we're signed in.

darzu · 2020-11-23T21:16:21Z

localtypings/projectheader.d.ts

@@ -28,7 +28,8 @@ declare namespace pxt.workspace {
        tutorialCompleted?: pxt.tutorial.TutorialCompletionInfo;


What is the difference between "Header" and "InstallHeader" ?

darzu · 2020-11-23T21:18:30Z

webapp/src/app.tsx

@@ -3090,6 +3036,10 @@ export class ProjectView
        }
    }

+    hasCloudSync() {
+        return this.isLoggedIn();


Note: maybe once we go to AAD we might not be able to store data ourselves and we might need OneDrive? Probably worth adding a TODO comment

darzu · 2020-11-23T21:18:40Z

webapp/src/app.tsx

@@ -4545,7 +4495,8 @@ document.addEventListener("DOMContentLoaded", () => {
    else if (isSandbox) workspace.setupWorkspace("mem");
    else if (pxt.winrt.isWinRT()) workspace.setupWorkspace("uwp");
    else if (pxt.BrowserUtils.isIpcRenderer()) workspace.setupWorkspace("idb");
-    else if (pxt.BrowserUtils.isLocalHost() || pxt.BrowserUtils.isPxtElectron()) workspace.setupWorkspace("fs");
+    //else if (pxt.BrowserUtils.isLocalHost() || pxt.BrowserUtils.isPxtElectron()) workspace.setupWorkspace("fs");


TODO uncomment

this disabled the filesystem workspace

darzu · 2020-11-23T21:20:45Z

webapp/src/cloud.ts

+}
+
+function deleteAsync(h: Header, prevVersion: Version, text?: ScriptText): Promise<void> {
+    return Promise.resolve();


add comment that this is intentionally left unimplemented ?

darzu · 2020-11-23T21:20:55Z

webapp/src/cloud.ts

+}
+
+function resetAsync(): Promise<void> {
+    return Promise.resolve();


TODO: need to implement

darzu · 2020-11-23T21:22:17Z

webapp/src/cloud.ts

+    return Promise.resolve();
+}
+
+export async function syncAsync(): Promise<any> {


modeled after: cloudsync.ts:syncAsync

bi-directional,
looks at local cloud project headers (with matching user IDs)
looks at remote headers
resolves them

darzu · 2020-11-23T21:23:43Z

webapp/src/cloud.ts

+                        workspace.saveAsync(file.header, file.text, file.version);
+                    } else {
+                        // Conflict.
+                        // TODO: Figure out how to register these.


Need to show cloud status on the cards

See Cloud Sync OneNote

Do we need timestamps or incremental version numbers? Or the user in the loop?
LastWrite wins?

hunch: auto-handling merges would be heavier lift and harder to get right than presenting options to user.

darzu · 2020-11-23T21:30:43Z

webapp/src/cloud.ts

+const FIELD_WORKING = "working";
+export const UPLOADING = `${MODULE}:${FIELD_UPLOADING}`;
+export const DOWNLOADING = `${MODULE}:${FIELD_DOWNLOADING}`;
+export const WORKING = `${MODULE}:${FIELD_WORKING}`;


note these are activity flags globally.. maybe we want per-project? TBD

darzu · 2020-11-23T21:31:38Z

webapp/src/cloudsync.ts

@@ -501,7 +503,7 @@ export function refreshToken() {
 }

 export function syncAsync(): Promise<void> {
-    return Promise.all([githubSyncAsync(), cloudSyncAsync()])
+    return Promise.all([githubSyncAsync(), cloud.syncAsync()])


note important change: instead of local cloudSyncAsync function, we're pointing to new cloud.syncAsync

darzu · 2020-11-23T21:33:00Z

webapp/src/package.ts

@@ -405,14 +405,14 @@ export class EditorPackage {
    }

    savePkgAsync() {


Notes:
Work to do to work with our cloud stuff?
Switch blobCurrent to cloudCurrent ?

When you create a new project, it currently doesn't sync to the cloud. This might be responsible.. TBD

darzu · 2020-11-23T21:33:59Z

webapp/src/projects.tsx

@@ -480,7 +475,7 @@ export class ProjectsCarousel extends data.Component<ProjectsCarouselProps, Proj
    }

    fetchLocalData(): pxt.workspace.Header[] {
-        const headers: pxt.workspace.Header[] = this.getData("header:*")


Note this was only fetching local headers... but it is called fetchLocal.. is it supposed to be local or cloud+local?

darzu · 2020-11-23T21:34:32Z

webapp/src/scriptmanager.tsx

@@ -184,11 +185,11 @@ export class ScriptManagerDialog extends data.Component<ScriptManagerDialogProps
                .then(text => workspace.duplicateAsync(header, text, res))
                .then(clonedHeader => {


called when you say "view all projects"

darzu · 2020-11-23T21:35:35Z

webapp/src/workspace.ts

@@ -25,14 +25,9 @@ type Header = pxt.workspace.Header;
 type ScriptText = pxt.workspace.ScriptText;
 type WorkspaceProvider = pxt.workspace.WorkspaceProvider;
 type InstallHeader = pxt.workspace.InstallHeader;


NOTE about this file: this operates on a layer agnostic of the specific workspace; it uses the interfaces

darzu · 2020-11-23T21:35:59Z

webapp/src/workspace.ts

+    let r = allScripts.map(e => e.header).filter(h =>
+        (withDeleted || !h.isDeleted) &&
+        !h.isBackup &&
+        (!h.cloudUserId || h.cloudUserId === cloudUserId))


note this is now responsible for including cloud projects if signed in

darzu · 2020-11-23T21:37:30Z

webapp/src/workspace.ts

+    let r = allScripts.map(e => e.header).filter(h =>
+        (withDeleted || !h.isDeleted) &&
+        !h.isBackup &&
+        (!h.cloudUserId || h.cloudUserId === cloudUserId))
    r.sort((a, b) => b.recentUse - a.recentUse)
    return r
 }


Some thoughts on merging two workspaces (instead of layer cloud on top of workspace-agnostic layer):

This was the path Sam and Peli took

Might be easier to work offline if we're not merging workspaces?

or maybe not?

darzu · 2020-11-23T21:41:13Z

webapp/src/workspace.ts

@@ -1352,21 +1335,6 @@ export function saveToCloudAsync(h: Header) {
    return cloudsync.saveToCloudAsync(h)
 }

-export function resetCloudAsync(): Promise<void> {


this wasn't used?

pelikhan · 2020-11-24T08:27:01Z

draft PR?

darzu · 2020-11-30T18:42:07Z

draft PR?

Yes, I accidentally created it as non-draft and I don't see a way to demote it :(

jwunderl · 2020-11-30T18:44:43Z

Under 'reviewers' there should be an option, it's a bit hidden (only shows as clickable on hover):

pelikhan · 2020-11-30T20:38:08Z

very hidden

…cAsync

…to cloud

darzu · 2021-02-20T00:57:04Z

moved here: #7902

wip

46c524c

darzu added the DO NOT MERGE label Nov 23, 2020

darzu commented Nov 23, 2020

View reviewed changes

localtypings/projectheader.d.ts Show resolved Hide resolved

darzu commented Nov 23, 2020

View reviewed changes

webapp/src/cloud.ts

}

function resetAsync(): Promise<void> {

return Promise.resolve();

Copy link

Contributor Author

darzu Nov 23, 2020

Choose a reason for hiding this comment

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

TODO: need to implement

darzu commented Nov 23, 2020

View reviewed changes

wip

cc33cf2

darzu added 25 commits December 12, 2020 10:57

lots of work on cloud sync

bc8a1a3

much better joint workspace

b8e8393

fix build issue

f8201ce

fix lint issues

6939562

lots of debugging

e6c3f9b

debugging

94605ea

horrible debugging

592c1ec

fix project transfer & debugging

7971ff3

fixining sync

88717cf

fix debug crash

d711126

debugging

71941fd

big progress on race conditions & sanity

ae41778

deprecate allscripts

be015c5

investigative comments and todos

e83fd6e

reworked headers hash (formerly last-mod-time)

8700481

todo list and progress on syncAsync

49fb645

return headers[] from sync; finer grain virtual api inval

6b388a8

better names

e970d53

unify and fix up maybeSyncHeadersAsync, refreshHeadersSession and syn…

bb676aa

…cAsync

undo typo

a62252d

debugging spurious saves

1d52319

first-save TODOs

a3155bd

debugging empty saves

d70f46a

fix issues with getTextAsync and lookup(); investigating double save …

0ea5813

…to cloud

handling 404s from the cloud

1e93dd1

darzu changed the title ~~[DRAFT] Client changes for cloud sync~~ [DRAFT] Workspace layer refactor Jan 4, 2021

darzu mentioned this pull request Jan 4, 2021

Project identity-based cloud synchronization #7743

Merged

darzu mentioned this pull request Feb 20, 2021

[DRAFT] Workspace layer refactor #7902

Draft

darzu closed this Feb 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] Workspace layer refactor #7657

[DRAFT] Workspace layer refactor #7657

darzu commented Nov 23, 2020 •

edited

Loading

darzu Nov 23, 2020 •

edited

Loading

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020 •

edited

Loading

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020

darzu Nov 23, 2020 •

edited

Loading

darzu Nov 23, 2020

pelikhan commented Nov 24, 2020

darzu commented Nov 30, 2020

jwunderl commented Nov 30, 2020

pelikhan commented Nov 30, 2020

darzu commented Feb 20, 2021

		@@ -28,7 +28,8 @@ declare namespace pxt.workspace {
		tutorialCompleted?: pxt.tutorial.TutorialCompletionInfo;

		@@ -405,14 +405,14 @@ export class EditorPackage {
		}

		savePkgAsync() {

		@@ -184,11 +185,11 @@ export class ScriptManagerDialog extends data.Component<ScriptManagerDialogProps
		.then(text => workspace.duplicateAsync(header, text, res))
		.then(clonedHeader => {

[DRAFT] Workspace layer refactor #7657

[DRAFT] Workspace layer refactor #7657

Conversation

darzu commented Nov 23, 2020 • edited Loading

darzu Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darzu Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darzu Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelikhan commented Nov 24, 2020

darzu commented Nov 30, 2020

jwunderl commented Nov 30, 2020

pelikhan commented Nov 30, 2020

darzu commented Feb 20, 2021

darzu commented Nov 23, 2020 •

edited

Loading

darzu Nov 23, 2020 •

edited

Loading

darzu Nov 23, 2020 •

edited

Loading

darzu Nov 23, 2020 •

edited

Loading