Should Promises still be used?

Question

And if yes - how?

I have the following IONIC 3 code and I dont know why it works how it works

First of all there is a so called "MusicService" which is responsible for loading the music files from the local storage:

private searchList: string[] = ["sdcard/Music", "sdcard/Download"]

public LoadMusicFromFS(): Promise<void>{
    this.foundMusic = []

    return new Promise((resolve, reject) => {
        this.searchList.forEach((dir)=>{
            this.CheckDir(dir, 0)
        })
    })
}

public foundMusic: Music[] = []
private CheckDir(dir: string, level: number): Promise<void>{
    return this.localFileSystem.listDir("file:///", dir).then((arr)=>{
        arr.forEach((elem)=>{

            if(elem.isDirectory){
                if(!(this.ignoreList.indexOf(elem.fullPath.slice(1, -1))>-1)) this.CheckDir(elem.fullPath.substr(1), level+1)
            }else{
                let fileType: string = elem.name.split(".").pop()
                if(this.searchTypes.indexOf(fileType)>-1){
                    console.log(elem.fullPath + " | " +elem.name+ " --> is Dir? "+elem.isDirectory)
                    let addingMusic: Music = new Music()
                    addingMusic.description.title = elem.name.split(".").slice(0, -1).join(".")
                    addingMusic.media.filePath = elem.fullPath
                    addingMusic.description.album.name="Test"
                    addingMusic.media.length=100



                    this.foundMusic.push(addingMusic)
                }
            }
        })
    }, err => {})
}

And there is also a Page where I call the function from the service:

this.platform.ready().then(() =>{
  this.musicService.LoadMusicFromFS().then(()=>{
    // This will never be printed -- why? 
    console.log("Music List successfully loaded")
  })
  // This loads the correct Music, but only because of references
  this.musicCache=this.musicService.foundMusic
})

I dont really get it - why isnt the "then" part working in the Page?

it is never printed probably because promise got rejected, you have to pass for example second callback to catch it when it gets rejected `then(()=>{ console.log("Music List successfully loaded") }, ()=>{ console.log("Music List error) })` — vardius, Mar 27 '18 at 00:15
You have to call the fist argument of your Promise within the Promise, for `.then()` to work. What is the use of a Promise that is not handling an Asynchronous action, anyways? — StackSlave, Mar 27 '18 at 00:28
@Vardius But it loads the stuff, so there shouldnt be an error or am I worng? — MauriceNino, Mar 27 '18 at 00:40
@PHPglue I dont really know what the use of it is, but the File plugin of IONIC forces me to use it — MauriceNino, Mar 27 '18 at 00:41
@MauriceNino yes it does load it because promise `CheckDir` is executed but promise `LoadMusicFromFS` is never resolved/rejected, check my answer there is no need to creating another promise here — vardius, Mar 27 '18 at 01:29

vardius · Accepted Answer · 2018-03-27T11:00:34.733

4

I recommend to read documentation this will help you better understand how promises work.

The executor normally initiates some asynchronous work, and then, once that completes, either calls the resolve function to resolve the promise or else rejects it if an error occurred. If an error is thrown in the executor function, the promise is rejected. The return value of the executor is ignored.

Your console log is never executed because never resolve/reject your promise

return new Promise((resolve, reject) => {
  this.searchList.forEach((dir)=>{
    this.CheckDir(dir, 0)
  })
})

You have to call resolve/reject for example

return new Promise((resolve, reject) => {
  // reject if searchList is empty
  if (this.searchList.length < 1) {
    reject();
  }
  Promise.all(this.searchList.map((dir)=> this.CheckDir(dir, 0))).then(
    () => resolve(),
    () => reject()
  );
})

The Promise.all(iterable) method returns a single Promise that resolves when all of the promises in the iterable argument have resolved or when the iterable argument contains no promises. It rejects with the reason of the first promise that rejects.

You could even do that:

public LoadMusicFromFS(): Promise<any[]>{
    this.foundMusic = []

    return Promise.all(this.searchList.map((dir)=> this.CheckDir(dir, 0)));
}

instead of wrapping it in another promise.

edited Mar 27 '18 at 11:00

answered Mar 27 '18 at 00:18

vardius

6,326
8
52
97

1

That's the wrong documentation though. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#Syntax – Bergi Mar 27 '18 at 00:36
2

`Promise.resolve(..)` is not the same as `resolve` function in the first parameter of `Promise` constructor. `Promise.resolve(..)`, as you cited, 'returns a Promise object that is resolved with the given value.' – Eric Mar 27 '18 at 00:39
Thanks. Notice that your proposed code *always* rejects the promise, though. – Bergi Mar 27 '18 at 00:43
Now it resolves/rejects when the *first* `CheckDir` has finished, not all of them. – Bergi Mar 27 '18 at 01:08
2

Thanks. Indeed the final snippet *should* be used, avoiding the [wrapping in a `new Promise` constructor](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Mar 27 '18 at 01:28
@Vardius thanks that worked out well! allthough that the return type of the function is not Promise but Promise – MauriceNino Mar 27 '18 at 10:32

score 0 · Answer 2 · answered Mar 27 '18 at 01:43

It looks like what you want to accomplish is to cache your result once the checkDir finishes.

That makes sense, and it's something you can potentially benefit by using Promise.

However, in your code:

this.musicCache = this.musicService.foundMusic

foundMusic is an array, so the assignment here merely assigns the reference to that array this.musicCache (i.e. shallow copy). I do not see the point of doing this. Maybe you want this.musicCache to be a map of some sort that maps the file path to the music object, or something like that?

Also, as per use of Promise, you can construct a value and resolve the promise to that value, and then then(..) it.

Should Promises still be used?

2 Answers2