Skip to content

Conversation

@rschamp
Copy link
Contributor

@rschamp rschamp commented Nov 12, 2018

Depends on scratchfoundation/scratch-vm#1750. Tests will fail until that PR is merged and this PR is bumped to the new version of the VM.

Known issues to be addressed in a follow-up:

Resolves

What Github issue does this resolve (please include link)?

Proposed Changes

Describe what this Pull Request does
Stop saving .sb3 files to the project server, and begin saving assets to the asset server and a project JSON file to the project server.

After assets are saved, mark them as "clean", and only save assets without this "clean" flag. This causes assets not to be saved multiple times unnecessarily.

Also, re-save the default costumes for the cat sprite. Because of line ending settings in Git, these files had diverged from their md5. To avoid this issue going forward, I set .gitattributes to consider all .svg files as binary, so git wouldn't worry about line endings in them.

Reason for Changes

Explain why these changes should be made
We want to save flat JSON files rather than zip files to the project server, and dedupe assets on the asset server. This also moves us closer to being able to track the project dirty state.

Test Coverage

Please show how you have added tests to cover your changes
Existing tests will pass, pending bumping the scratch-vm dependency above.

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

  • Chrome
  • Firefox
  • Safari

Windows

  • Chrome
  • Firefox
  • Edge

Chromebook

  • Chrome

iPad

  • Safari

Android Tablet

  • Chrome

Copy link
Contributor

@benjiwheeler benjiwheeler left a comment

Choose a reason for hiding this comment

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

I can't seem to try this out without causing errors:

Uncaught TypeError: Cannot read property 'click' of undefined
    at SBFileUploader.handleClick (webpack:///../scratch-gui/dist/scratch-gui.js?:293736)
gui Unhandled Error: Error: A cross-origin error was thrown. React doesn't have access to the actual error object in development. See https://fb.me/react-crossorigin-error for more information.    at Object.invokeGuardedCallbackDev (webpack:///./~/react-dom/cjs/react-dom.development.js?:588:19)

What I'm doing:

  1. edit project I own
  2. click on the File menu

@benjiwheeler
Copy link
Contributor

benjiwheeler commented Nov 13, 2018

Tests are failing -- looks like project-saver-hoc

BTW I reran them in case it was just a passing problem

@rschamp rschamp changed the title Feature/incremental saves Save 3.0 project JSON to the project server and assets to the assets server Nov 14, 2018
@rschamp
Copy link
Contributor Author

rschamp commented Nov 14, 2018

Tests will fail until the VM PR is merged and updated here. This depends on vm.assets, which won't exist until that's merged.

The cross-origin error is usually a secondary error from a 500 (or some other error) response, which don't have CORS headers set on it. Can you check out the network tab? Maybe your local API or projects servers need to get updated?

@rschamp
Copy link
Contributor Author

rschamp commented Nov 14, 2018

@benjiwheeler I just realized I forgot to push and make a PR for the assets server, which needs updating for this to work. That's probably your CORS issue, sorry!

@benjiwheeler
Copy link
Contributor

Got this to run without crashing, using that -assets PR!

How else should I test it? It saves without crashing after I mess around with costumes... anything else I can try?

Ray Schamp added 4 commits November 15, 2018 10:50
These files have diverged from the versions stored on the assets server due to the way Git handles line endings. Re-save them and keep their line endings from changing again by treating SVGs as binary in this repo.
Instead of saving .sb3 files to the projects server, save just the project JSON. Save assets to the assets server. This happens via the asset types being routed to different web sources by storage.
Hopefully they are actually clarifying
@rschamp rschamp force-pushed the feature/incremental-saves branch from a22aeb2 to 71baad2 Compare November 15, 2018 10:50
@rschamp
Copy link
Contributor Author

rschamp commented Nov 15, 2018

@benjiwheeler Let's follow up on the comment. I would like this in for the freeze.

@rschamp rschamp merged commit 2f4f8a1 into scratchfoundation:develop Nov 15, 2018
@rschamp rschamp deleted the feature/incremental-saves branch November 15, 2018 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mark saved assets as "not dirty" after saving them

3 participants