NativeScript / NativeScript

⚡ Empowering JavaScript with native platform APIs. ✨ Best of all worlds (TypeScript, Swift, Objective C, Kotlin, Java, Dart). Use what you love ❤️ Angular, Capacitor, Ionic, React, Solid, Svelte, Vue with: iOS (UIKit, SwiftUI), Android (View, Jetpack Compose), Dart (Flutter) and you name it compatible.
https://nativescript.org
MIT License
24.07k stars 1.64k forks source link

Improve Error handling of file-system module [Android & iOS] #3309

Open kazemihabib opened 7 years ago

kazemihabib commented 7 years ago

Hi, 1) https://docs.nativescript.org/api-reference/classes/_file_system_.folder.html#parent says that Gets the Folder object representing the parent of this entity. Will be null for a root folder like Documents or Temporary. This property is readonly.

but if you test

let myRoot = fs.File.fromPath('/');
let parent = myRoot.parent;

It will throw error.( in docs says it will return null). because the java api returns null for the parent so in the below line error happens

https://github.com/NativeScript/NativeScript/blob/master/tns-core-modules/file-system/file-system-access.android.ts#L18

Then in the catch it calls the onError function that it throws an error and in the next line it returns undefined when error happens (it seems useless because it throws error first) So we should handle this like this

try{
        let myRoot = fs.File.fromPath('/');

        let parent = myRoot.parent;
}catch(error){
            console.log('inAppcomponent ', error);
        }

I don't know which one was the target , docs or implementation.

2) but I think the file-system really needs some changes ,especially with error handling methods.

3)In file-system and in other parts there are throw something that are not mentioned in docs might throw error.

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/40152848-improve-error-handling-of-file-system-module-android-ios?utm_campaign=plugin&utm_content=tracker%2F12908224&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F12908224&utm_medium=issues&utm_source=github).
NickIliev commented 7 years ago

Hi @kazemihabib

File.fromPath() is expecting a path with write permissions ending with the file name.

e.g.

var documents = fs.knownFolders.documents();
var path = fs.path.join(documents.path, "FileFromPath.txt");
var file = fs.File.fromPath(path);

You can not create a file with fs.File.fromPath('/');

Perhaps you want to create a folder but then you need to use

fs.Folder.fromPath()

Still, you will have to provide a name for the folder you are creating and a path within the application where you will have write permissions.

kazemihabib commented 7 years ago

I tested fs.Folder.fromPath() too, it throws the same error JS: TypeError: Cannot read property 'getAbsolutePath' of null because of the reason above.

NickIliev commented 7 years ago

Hey @kazemihabib you should provide a path to the method fromPath and this should be a valid path. For both Android and iOS we have exposed some paths via knownFolders :

    var documents = fs.knownFolders.documents();
    var currentApp = fs.knownFolders.currentApp();
    var temp = fs.knownFolders.temp();

    // var path = fs.path.join(documents.path, "myFolderName");
    let myRootDoc = fs.Folder.fromPath(documents.path);
    let myRootApp = fs.Folder.fromPath(currentApp.path);
    let myRootTmp = fs.Folder.fromPath(temp.path);

    console.log("myRootDoc: " + myRootDoc.path); // returns (Android) something like /data/user/0/org.nativescript.issue3309/files
    console.log("myRootDoc.parent: " + myRootDoc.parent.path);

    console.log("myRootApp: " + myRootApp.path); //  returns (Android) something like /data/user/0/org.nativescript.issue3309/app
    console.log("myRootApp.parent: " + myRootApp.parent.path);

    console.log("myRootTmp: " + myRootTmp.path); //  returns (Android) something like /data/user/0/org.nativescript.issue3309/cache
    console.log("myRootTmp.parent: " + myRootTmp.parent.path);

This said you will need to provide a relative path and providing "/" will return undefined as the file system can not resolve such path nor in iOS or Android. If you need access to external folders then you can get it with using the native APIs. Here are some of the relative paths that can be accessed in Android.

    var downloads = android.os.Environment.getExternalStoragePublicDirectory(android.os.Environment.DIRECTORY_DOWNLOADS).toString();
    let androidDownloadsPath = fs.Folder.fromPath(downloads);
    console.log("androidDownloadsPath: " + androidDownloadsPath.path);
    console.log("androidDownloadsPath.parent: " + androidDownloadsPath.parent.path);

However, you should check if you have permissions to access the folders and if the permissions are read-only or read/write.

And yet, indeed there are some places where the error login can be improved for the file-system module. If you have implementations in mind you can join us as a contributor. (if interested check this link)

kazemihabib commented 7 years ago

Hi @NickIliev , let me explain the problem again. (I tested this in android) example:

   let root = fs.Folder.fromPath('/');
    console.log('root path ',root.path); // root path  /

    try {
        let parent = root.parent;
        console.log('parent', parent); // error thrown in above line not reaches to this :(
    } catch (error) {
        console.log('error with root.parent', error); //error with root.parent TypeError: Cannot read property 'getAbsolutePath' of null
    }

Your sentence : This said you will need to provide a relative path and providing "/" will return undefined as the file system can not resolve such path nor in iOS or Android As I show in above code I can access the '/' path (It logs the root path / correctly).

so what was the problem that I opened this ISSUE:

look at the let parent = root.parent; in this line it throws error that should be handled.

why it throws error?

https://github.com/NativeScript/NativeScript/blob/master/tns-core-modules/file-system/file-system-access.android.ts#L18

because the native api var javaFile = new java.io.File(path); var parent = javaFile.getParentFile(); returns null for parent of '/' . and in the next line return { path: parent.getAbsolutePath(), name: parent.getName() }; it tries to getAbsolutePath of the null. it throws Cannot read property 'getAbsolutePath' of null and this error is handles by try catch.

  try {
           var javaFile = new java.io.File(path);
           var parent = javaFile.getParentFile();

           return { path: parent.getAbsolutePath(), name: parent.getName() };
        } catch (exception) {
          // TODO: unified approach for error messages
          if (onError) {
              onError(exception);
        }

        return undefined;
 } 

and it throws the error in onError callback https://github.com/NativeScript/NativeScript/blob/master/tns-core-modules/file-system/file-system.ts#L61

It would be really better if it returns the null in nativescript too (like the native api of android) or instead of throwing error , use error handling with callback.

Contribution

I like to contribute but first I need to know if others have same opinion about this problem and error handling of this module. and two months ago I added some useful methods to this module and other modules but I never send pull request because they were for Android , soon I will study little about obj-c and ios development to prepare those methods for ios too.