First pass at multiple projects with temporary UI #644
Conversation
|
Preview build will be at |
This option is probably going away.
microbit-matt-hillsdon
left a comment
microbit-matt-hillsdon
left a comment
There was a problem hiding this comment.
WIP review but making comments visible as I go
src/storage.ts
Outdated
| } | ||
|
|
||
| async removeModel() { | ||
| console.trace("when"); |
There was a problem hiding this comment.
| console.trace("when"); |
src/storage.ts
Outdated
| const settings = assertData( | ||
| await settingsStore.get(DatabaseStore.SETTINGS) | ||
| ); | ||
| makeCodeData.project.header?.name; |
There was a problem hiding this comment.
Was this intended to do anything?
There was a problem hiding this comment.
Unclear. Likely leftover from debugging and I can't think of what else it would be needed for so removing 3eb2d37
src/storage.ts
Outdated
| return tx.done; | ||
| } | ||
|
|
||
| async deleteProject(id: string): Promise<PersistedProjectData | boolean> { |
There was a problem hiding this comment.
If we need all the info being returned here I think we should model it more clearly.
Or perhaps some of the responsibility belongs elsewhere? I'm a bit unsure about the storage layer having an idea of the current project.
There was a problem hiding this comment.
Probably worth talking this bit through. It's messy in the method itself and where it is called.
We can easily move away from the storage layer having an idea of the current project. For a long time the projectId was isolated to the storage layer, but it's now required in state to react to model changes.
src/storage.ts
Outdated
| const localStorageProject = getLocalStorageProject(); | ||
| if (localStorageProject) { | ||
| const model = await tf.loadLayersModel(oldModelUrl); | ||
| if (model) { |
There was a problem hiding this comment.
Is this definitely right? The type definitions suggest this doesn't return undefined/null and so might instead throw. Worth a comment if it is correct. There's also one other use.
There was a problem hiding this comment.
See loadModelFromStorage which is handling this error at some distance from its cause.
There was a problem hiding this comment.
Yes, it's return a model or throw. I've moved the error handling closer to the function calls for all model storage actions e786ff1
| } | ||
| }; | ||
|
|
||
| useStore.subscribe(async (state, prevState) => { |
There was a problem hiding this comment.
Feels like it's worth doing multiple model storage soon so we don't have this hard to reason about code dealing with the only thing that isn't per project.
There was a problem hiding this comment.
This version does have multiple model storage. The projectId check prevents your current model from being removed when you start a new session. The flow at things stand:
- Existing project with existing model (all saved in IndexedDB)
- Start a new session
- This updates state first with a new empty project and new project id
- The subscription callback runs and without the project id check, the existing model is cleared which we don't want
- The storage layer is then called with a new project and new project id
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Simplify project deletion. Update project updatedAt for appropriate route changes.
When opening MakeCode, simply close other instances in other tabs/ windows
This is a first pass at storing multiple projects and builds on the storage branch which transitions from using localStorage to IndexedDB. There may well be bugs in this implementation, please document these if you hit them during testing.
If additional code changes are made, you will probably need to manually delete the "ml" database in IndexedDB using devtools.
TODO: