Skip to content

feat(file-system): async read/write #7671

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.

Sign up for GitHub

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

Jump to bottom
Merged
manoldonev merged 13 commits into from
Oct 2, 2019
Merged

feat(file-system): async read/write #7671

manoldonev merged 13 commits into from
Oct 2, 2019

Conversation

@PeterStaev
Copy link
Contributor

@PeterStaev PeterStaev commented Aug 11, 2019 •
edited
Loading

PR Checklist

What is the current behavior?

Current file-system methods for read/write files are synchronous which makes the UI thread block if a big file is read/written.

What is the new behavior?

True async methods have been added that use native code inside tns-core-modules-widgets execute the read/write operations in a separate thread.

Closes #2239 .

@cla-bot cla-bot bot added the cla: yes label Aug 11, 2019
@PeterStaev PeterStaev changed the title feat(files-system): async read/write feat(file-system): async read/write Aug 11, 2019
@NathanaelA

This comment was marked as abuse.

Sign in to view
@PeterStaev
Copy link
Contributor Author

PeterStaev commented Aug 12, 2019

Yes, this is a possible solution, if breaking changes in minor versions are considered that bad :)

@etabakov
Copy link
Contributor

etabakov commented Aug 13, 2019

Hey, @PeterStaev. Regarding the breaking changes policy, our team is committed to following the SemVer Specification, which means that all changes in a minor version should be introduced in a backward-compatible manner. With that said, Nathanael's suggestion is a good one. Would you mind adjusting your PR according to his proposal, so that we can proceed with the technical review?

Thanks for jumping on this issue!

@PeterStaev
Copy link
Contributor Author

PeterStaev commented Aug 13, 2019

@etabakov , updated and removed the breaking changes. Removed from the above description as well :)

@manoldonev manoldonev self-requested a review August 21, 2019 13:53
@isabelh340
Copy link

isabelh340 commented Sep 10, 2019

Alot

manoldonev
manoldonev reviewed Sep 18, 2019
View reviewed changes
manoldonev
manoldonev reviewed Sep 18, 2019
View reviewed changes
manoldonev
manoldonev reviewed Sep 19, 2019
View reviewed changes
@manoldonev
Copy link
Contributor

manoldonev commented Oct 1, 2019

test

1 similar comment
@SvetoslavTsenov
Copy link
Contributor

SvetoslavTsenov commented Oct 1, 2019

test

@manoldonev manoldonev added the docs needed Additional documentation on this issue/PR is needed label Oct 2, 2019
@manoldonev manoldonev merged commit 2146ac9 into NativeScript:master Oct 2, 2019
@manoldonev
Copy link
Contributor

manoldonev commented Oct 2, 2019

@PeterStaev Looking good now and I have merged the PR. Thanks!
Will check what exactly is the process with https://www.bountysource.com/issues/34832190-make-the-file-system-module-run-in-a-background-thread and someone with the NativeScript team will follow up with you.

@manoldonev manoldonev mentioned this pull request Oct 2, 2019
Closed
@PeterStaev PeterStaev deleted the issue-2239 branch October 2, 2019 15:09
@vakrilov vakrilov mentioned this pull request Oct 25, 2019
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@manoldonev manoldonev manoldonev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes ♥ community PR docs needed Additional documentation on this issue/PR is needed

Projects

NativeScript Docs Project
Status: 🆕 New

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make the file-system module run in a background thread [$300]

7 participants

@PeterStaev @NathanaelA @etabakov @isabelh340 @manoldonev @SvetoslavTsenov @VladimirAmiorkov