Importer: Supporting ZIP Imports#3328
Conversation
| } | ||
|
|
||
| if (endsWith(fileName, '.json')) { | ||
| this.processJsonFile(file); |
There was a problem hiding this comment.
JSON Import logic has been extracted into processJsonFile
| }); | ||
| }; | ||
|
|
||
| processZipFile = (file) => { |
There was a problem hiding this comment.
And this would be the new processor!
| const fileName = file.name.toLowerCase(); | ||
|
|
||
| // Limit file size we will read to 5mb | ||
| if (file.size > 5000000) { |
There was a problem hiding this comment.
deary me. why do we limit imports to 5 MB?
There was a problem hiding this comment.
Not sure, but... perhaps just as a failsafe? (nobody ever complained about this AFAIK)
| this.recordEvent('importer_import_completed', { | ||
| source: 'simplenote', | ||
| note_count: noteCount, | ||
| import(/* webpackChunkName: 'jszip' */ 'jszip') |
There was a problem hiding this comment.
nice job lazy-loading this module. was this library already included in the project?
There was a problem hiding this comment.
YES! there's a snippet just like this in to-zip.js
There was a problem hiding this comment.
coooool. can we verify that this lazy-loads and isn’t pulled in as soon as the app loads? I think we should be able to do that via the dev server and the local browser dev tools
There was a problem hiding this comment.
I'm only seeing this line after clicking Import in Settings > Tools. I believe we're good!
| const mockFileReader = new FileReader(); | ||
| (FileReader as any).mockImplementation(() => mockFileReader); | ||
|
|
||
| importer.processZipFile(zipFile); |
There was a problem hiding this comment.
this could be calling importer.importNotes() as in the previous test, but as it is, it’s reaching into an internal method making the test brittle and at risk of diverging from the actual app code
| .then((zip) => { | ||
| // Look for JSON files in the ZIP | ||
| const jsonFiles = Object.keys(zip.files).filter((filename) => | ||
| filename.toLowerCase().endsWith('.json') |
There was a problem hiding this comment.
we can look specifically for source/notes.json since that’s what the official exporter produces.
we don’t want ZIPs with random JSON destroying accounts with junk data
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
From a pairing review session
|
@dmsnell Ready for another look, thank you! |
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
Ready for merge and deployment. We should be ready and reactive when we do so, however, to ensure that we don’t break things accidentally.
What testing have you performed on these changes? I have performed none.
|
I've tested the steps outlined in the PR (importing a ZIP!). Let's go forwards, reverting takes a sec, if anything. Thank you Dennis!! |
2 participants
Fix
In this PR we're enhancing the Importer, so that users can import the
zipfiles, exactly the way they've exported them.Test
npm run devhttp://localhost:4000/Settings > Tools > Import Notesnpm test)Release
RELEASE-NOTES.mdwas updated with: