PrismarineJS / prismarine-schematic

Read and write schematic of any minecraft version, provide them in a convenient and stable API
https://prismarine.js.org/prismarine-schematic/
MIT License
21 stars 8 forks source link

Add mapSync and forEachSync #37

Closed IceTank closed 3 years ago

u9g commented 3 years ago

idk about this, imo we should just change map and foreach to do what they would usually do

IceTank commented 3 years ago

That would be a breaking change then. I don't know why the forEach and map were async functions in the first place but the names like this would mimic the fs library with writeFile being async and writeFileSync being synchronous.

extremeheat commented 3 years ago

Why do you need it to be a synchronous function? You can already chain .then if you don’t want to use await when returning a promise. Doing await on a sync function completely ignores the modifier. I think the name is also a bit confusing, because the callback calls now become asynchronous if they return promises.

IceTank commented 3 years ago

I want a function that is synchronous in its execution. If I use forEach to add up a counter I would have to await forEach. It looks like putting an await keyword in front of a synchronous function inside a async function does not mean that it executes in order. I tested it with this code on node v14.15.3:

const result = []
function syncFun(i) {
    result.push(i)
}
async function f1() {
    for (let i = 0; i < 1000; i++) {
        await syncFun(i)
    }
    console.info('Finished')
    return result
}
function start() {
    console.info('Start')
    f1()
    console.info('End')
}
start()

It prints:

Start
End
Finished

It does not wait for f1() to finish. And instead continues while f1() is still running. So I guess the same would happen if you use forEach without awaiting the promise? If I remove the await in front of the syncFun() call it works however.

u9g commented 3 years ago

I want a function that is synchronous in its execution. If I use forEach to add up a counter I would have to await forEach. It looks like putting an await keyword in front of a synchronous function inside a async function does not mean that it executes in order. I tested it with this code on node v14.15.3:

const result = []
function syncFun(i) {
    result.push(i)
}
async function f1() {
    for (let i = 0; i < 1000; i++) {
        await syncFun(i)
    }
    console.info('Finished')
    return result
}
function start() {
    console.info('Start')
    f1()
    console.info('End')
}
start()

It prints:

Start
End
Finished

It does not wait for f1() to finish. And instead continues while f1() is still running. So I guess the same would happen if you use forEach without awaiting the promise? If I remove the await in front of the syncFun() call it works however.

Can you explain what you are trying to achieve that the current implementation doesn't work for?

IceTank commented 3 years ago

Can you explain what you are trying to achieve that the current implementation doesn't work for?

Have a synchronous execution version of forEach. The current forEach version has to be used with await to have a sequential execution. If you are not using await forEach will exit on the next promise micro task loop. So if you want to use forEach to add up an amount of blocks you would would have to do it in a asynchronous function as you have to await the forEach result.

extremeheat commented 3 years ago

Just use await. I explained the issue with your example above on discord.