Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zip import/export (based from zip-import-export branch) #80

Merged
merged 14 commits into from
Jun 26, 2024

Conversation

jsoules
Copy link
Collaborator

@jsoules jsoules commented Jun 25, 2024

This is a modification of @magland's zip-import-export branch (PR #53) to use the amended data model from PR #77.
UI features are unchanged from that PR, so I'll refer to that for description, and this write-up will focus on the code changes (particularly as distinct from that PR/branch).

Most of the changes are to back-end files located under the gui/src/app/SPAnalysis directory. Front-end changes are under gui/src/app/pages/HomePage/. A few other smaller changes are in other directories and I'll describe them with full paths.

--
Probably the biggest focus of this PR is to establish standardized filesystem representations for the parts of the overall data model (SPAnalysisDataModel). This mapping is mainly implemented in FileMapping.ts. I've also created a new file for serialization code in SPAnalysisSerialization.ts. The reducer also had some significant changes to handle some new ways of updating the data model.

On the front-end side, code is mostly the same as in @magland's branch. I made some small changes to the ExportWindow.tsx and ImportWindow.tsx components to move some of their logic into the back-end code section (incl. the reducer), but as this is conceived as an update to that branch I tried to leave most of the functionality untouched.

Additionally, I've added replaceSpaces.ts and triggerDownload.ts functions in the util directory; these are small and straightforward.

Here are some notes on the back-end changes:

  • FileMapping.ts: Probably the trickiest part, but is commented pretty extensively.
    • Defines an enum for filesystem filenames (e.g. main.stan, data.json, etc.)
    • Defines a mapping from filenames to data-model field names.
    • Defines a type that maps filenames to filename contents.
    • Function to map a data model to a (partial) list of persistent files.
    • Function to take a (partial) list of persistent files and map their contents to a data model.
  • SPAnalysisReducer:
    • Add a new action type that takes a partial list of data model fields & maps their contents to an instance of the data model.
    • Added functions to update data model metadata, data model sampling opts, and other fields (the "files" fields, which are text blobs).
    • All above functions have an option to determine whether or not to preserve any existing data that isn't being touched.
  • SPAnalysisSerialization: New file.
    • Move the "serialize everything to local storage" functions (which had been in the data model file).
    • Moved the zip serialization/deserialization code here from where it was in PR frontend: import/export #53 (on the UI components).
  • SPAnalysisDataModel:
    • Added type exports, including one for the data model with the "ephemera" key omitted.
    • Added utility functions for copying permanent state to the ephemera, for returning appropriately stringified fields from the data model object, for looking up defined-field keys, and tracking whether models have been changed.
  • SPAnalysisContextProvider: trivial changes.

Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

A few misc comments/questions

gui/package.json Outdated Show resolved Hide resolved
gui/src/app/SPAnalysis/SPAnalysisDataModel.ts Outdated Show resolved Hide resolved
gui/src/app/util/triggerDownload.ts Outdated Show resolved Hide resolved
gui/src/app/util/replaceSpaces.ts Outdated Show resolved Hide resolved
gui/src/app/SPAnalysis/SPAnalysisDataModel.ts Outdated Show resolved Hide resolved
@magland
Copy link
Collaborator

magland commented Jun 25, 2024

Tried in a codespace and cot this error

 [ERROR] No matching export in "src/app/util/replaceSpaces.ts" for import "replaceSpaces"

    src/app/SPAnalysis/SPAnalysisSerialization.ts:2:9:
      2 │ import { replaceSpaces } from "../util/replaceSpaces"
        ╵          ~~~~~~~~~~~~~


    at failureErrorWithLog (/workspaces/stan-playground/gui/node_modules/esbuild/lib/main.js:1472:15)
    at /workspaces/stan-playground/gui/node_modules/esbuild/lib/main.js:945:25
    at runOnEndCallbacks (/workspaces/stan-playground/gui/node_modules/esbuild/lib/main.js:1315:45)
    at buildResponseToResult (/workspaces/stan-playground/gui/node_modules/esbuild/lib/main.js:943:7)
    at /workspaces/stan-playground/gui/node_modules/esbuild/lib/main.js:955:9
    at new Promise (<anonymous>)
    at requestCallbacks.on-end (/workspaces/stan-playground/gui/node_modules/esbuild/lib/main.js:954:54)
    at handleRequest (/workspaces/stan-playground/gui/node_modules/esbuild/lib/main.js:647:17)
    at handleIncomingPacket (/workspaces/stan-playground/gui/node_modules/esbuild/lib/main.js:672:7)
    at Socket.readFromStdout (/workspaces/stan-playground/gui/node_modules/esbuild/lib/main.js:600:7)

I think there was a function name change?

@jsoules
Copy link
Collaborator Author

jsoules commented Jun 25, 2024

Tried in a codespace and got this error
[...]
I think there was a function name change?

Yep, I committed the change direct from the comment and missed that it needed to be corrected where it is imported as well. Should be fixed now.

Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

This looks good to me! I left a couple comments, none of which are blockers on a merge IMO

Comment on lines +56 to +60
// The user has uploaded a single file and it is not a zip file. In
// this case we want to give the user the option whether or not to
// replace the current project.
setShowReplaceProjectOptions(true)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy to leave this until after merge, just echoing my discussion from #53 that I think uploading a single file should keep the drop box enabled to allow you to also add a second/third etc

gui/yarn.lock Outdated Show resolved Hide resolved
@WardBrian WardBrian merged commit 17211fc into main Jun 26, 2024
1 check passed
@WardBrian WardBrian deleted the zip-import-export-js branch June 26, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants