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

Add SimplenoteImporter #975

Merged
merged 3 commits into from
Nov 2, 2018
Merged

Add SimplenoteImporter #975

merged 3 commits into from
Nov 2, 2018

Conversation

mirka
Copy link
Member

@mirka mirka commented Oct 30, 2018

Depends on #922

  • Is the class name good? Should it be called SimplenoteJSONImporter? 🤔
  • This assumes that the import will be more or less instantaneous once the JSON file is read and parsed. (If this is insufficient, we might want to consider emitting the progress status events from CoreImporter instead of the source-specific parsers?)

@mirka mirka added this to the 1.3.0 milestone Oct 30, 2018
@mirka mirka requested a review from roundhill October 30, 2018 16:04
}

// Limit file size we will read to 2mb
if (file.size > 2000000) {
Copy link
Contributor

@roundhill roundhill Oct 30, 2018

Choose a reason for hiding this comment

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

I have a Simplenote export file with 1,600 notes that is just over 2mb. Maybe we should set a 5mb limit? Not sure if you tested it with a large export or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Today I tested with export files of varying sizes (80KB – 4MB). While large files did not choke the app in terms of memory, it did degrade the experience in non-trivial ways:

  1. There is considerable lag between the point when all notes have been added to the noteBucket, and the point when the notes appear (all at once) in the Note List.
  2. Clicking on notes in the Note List is unresponsive for a while. The onClick is firing, but the redux dispatch is being deferred until a certain point... I'm not immediately sure what is causing this.
  3. It takes quite a while for all the notes to be synced over to the server, and it is almost impossible to tell when it has finished. The only way to tell is to try clicking the Log Out button and see if it throws an unsynced notes warning.
  4. If there are lots of notes AND lots of tags, the tags take a long time to show up in the tag drawer.

With these constraints, I felt like a 500-note (~400KB) export file was as far as I wanted to go... 😵 And since these issues are occurring after the CoreImporter is done, I'm guessing the other importers will encounter them too.

I'll see if there are some easy things we can do to address at least 1 and 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

^ Addressed in #986 #987 #988

@roundhill
Copy link
Contributor

  • Is the class name good?

It WFM 👍

@roundhill
Copy link
Contributor

This will need to be based on master now that we've merged the core importer.

@mirka mirka changed the base branch from feature/import-parser to master November 2, 2018 05:56
@mirka mirka force-pushed the feature/simplenote-parser branch from 103caa1 to 76dbbe3 Compare November 2, 2018 06:06
@mirka mirka merged commit 6bcc12c into master Nov 2, 2018
@mirka mirka deleted the feature/simplenote-parser branch November 2, 2018 07:35
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.

2 participants