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

Fix large asset import on Cordova #692

Merged
merged 3 commits into from
Oct 5, 2018
Merged

Fix large asset import on Cordova #692

merged 3 commits into from
Oct 5, 2018

Conversation

bryfox
Copy link
Contributor

@bryfox bryfox commented Oct 2, 2018

Fixes #684. (Android & iOS.)

Test scenario:

  1. Create a protocol package from the development protocol source
    • Rename the protocol by changing the "name" property in protocol.json
    • Zip up contents & rename to *.netcanvas
  2. Import this file to Server
  3. Open NC in Android/iOS (actual device)
  4. Pair with Server if needed
  5. Import the created protocol
  6. Wait

Expected: protocol is eventually imported successfully & displayed. Also note that (assuming you renamed the protocol in step 1) new sessions created from the imported protocol start quickly as expected.

Known issues:

  • It's slow [on large assets]
    • Could be worth experimenting with increasing the buffer size to reduce write calls, but this increases memory use, so there would have to be some happy medium we derive from actual device testing.
    • Could theoretically rewrite zip extraction natively
    • Could be a good place for a progress bar, and/or textual feedback like "Extracting {fileName.ext}..."
    • ...keeping in mind that importing is likely a relatively uncommon action?
  • Implementation is hard to test. I stuck with the pattern of other filesystem functions, but would refactoring to a new class make sense now or in the future?

bryfox added 3 commits October 2, 2018 16:13
Despite not returning a bytesWritten value, the fileWriter doesn't
guarantee that all bytes are written.
@wwqrd
Copy link
Contributor

wwqrd commented Oct 3, 2018

Works for me on an iOS device.

I stuck with the pattern of other filesystem functions, but would refactoring to a new class make sense now or in the future?

What would this look like? If we can separate out the platform implementations at the same time we could move to native zip too?

@bryfox
Copy link
Contributor Author

bryfox commented Oct 3, 2018

I'm not sure exactly what a refactor would look like; I was thinking extracting to a class & breaking it up to make some of the functions more testable. On the other hand, this is really just glue between JSZip and FileWriter, so despite function length, it might be easier to read/understand as it is? Integration tests would probably be more useful than unit tests anyway, but I don't know if we'll have that for cordova any time soon.

If the slowness is a problem, moving to native zip could be worth testing. I'd probably start with buffer size adjustments and compare that to native before starting on a new plugin.

@bryfox
Copy link
Contributor Author

bryfox commented Oct 5, 2018

I'd like to treat performance improvements mentioned here as a separate issue (whether part of the next release or not); a 25MB import is slow, but it was faster before mostly because it was crashing halfway through.

@bryfox bryfox merged commit 875f940 into master Oct 5, 2018
@bryfox bryfox deleted the fix/large-protocol-load branch October 5, 2018 16:32
@jthrilly
Copy link
Member

jthrilly commented Apr 5, 2019

Relates to #881

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