feat: create addHelper to add helper w/ priority
#55
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The list of helpers in `ScratchStorage` is no longer fixed: now it's possible to add a new helper using the `addHelper` method. Helpers are sorted by their relative priority values, which are provided at add time. As a follow-up task, `WebHelper` should probably be adjusted to represent only one endpoint, with create/update/delete features for that one endpoint. A normal Scratch session might then have one `WebHelper` for projects and a second for images and sounds. The main reason to do this would be to concentrate the "try the next place" logic in `ScratchStorage` instead of having a second version of it in `WebHelper`.
This new test verifies that helpers are used in the correct order, corresponding to the priority value provided to `addHelper`.
rschamp
previously requested changes
Oct 25, 2018
Contributor
rschamp
left a comment
rschamp
left a comment
There was a problem hiding this comment.
The tests are a little hard to read and have state shared between them to function. Not a super big deal but would be nice to fix (as long as that's what's happening!)
test/unit/helper-priority.js
Outdated
| test('addHelper', t => { | ||
| storage.addHelper(new FakeHelper('third', true), -50); | ||
| storage.addHelper(new FakeHelper('first', false), 50); | ||
| storage.addHelper(new FakeHelper('second', false), 0); |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
test/unit/helper-priority.js
Outdated
|
|
||
| test('priorities', t => { | ||
| t.deepEqual(logEntries, []); | ||
| return storage.load('fakeAssetType', 'fakeAssetId', 'fakeDataFormat').then(() => { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
cwillisf
force-pushed
the
storage-helper-priorities
branch
from
2f81295 to
970ee74
Compare
These changes break the `addHelper` tests into several phases which don't share state, and hopefully improve readability of the tests.
cwillisf
force-pushed
the
storage-helper-priorities
branch
from
970ee74 to
c7bba01
Compare
rschamp
approved these changes
Nov 1, 2018
Contributor
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
This change creates a new
addHelpermethod, used to add a Helper to the storage module. Every registered Helper now has a priority value, and Helpers are consulted in descending priority order during a load.Reason for Changes
This allows
scratch-desktopto inject its own Electron-specific Helper into the storage system. In the longer term I think this will also allow us to simplify the storage module, and in particular sets us up to eliminate some redundant code inScratchStorage.jsandWebHelper.js.Followup
If this PR is merged I will file these as followup tasks:
storemethod should support multiple Helpers, consulted in priority order like in the load case.webHelperproperty onScratchStorageshould be removed.storemethod on a Helper should distinguish between "I don't handle this kind of asset" (perhaps returningnull) and "I tried to handle this and failed" (perhaps returning and rejecting a Promise). In the former case we should move on to the next helper; in the latter we should probably fail the overallstorecall.AssetTypefiltering logic fromWebHelperintoScratchStorage.WebHelpershould only handle one "store" and should not contain failover logic.WebHelperinstances instead.loadmethod onWebHelpershould be simplified to removetryNextSource: instead it should rely ontryNextHelperinScratchStorage.Test Coverage
Please show how you have added tests to cover your changes