chore: Formatted with prettier

[mondoreale]

Summary

Format all the codes using Prettier and its refreshed config from prettier branch.

@teogeb, attached is the result of npm run eslint with indent enabled.

I've tweaked the branch so that it's the only outstanding rule we have to deal with.

(Note that in this PR the indent rule is on.)

Cheers!

Changes

• Auto-formatted all the files.
• Adjusted selected eslint rules to match Prettier's behaviour:
  • Enabled avoidEscape on the quotes rule.
  • Changed @stylistic/member-delimiter-style to semi for singleline types and interfaces (semi is typescript's default, too).

Limitations and future improvements

• n/a

Checklist before requesting a review

• Is this a breaking change? If it is, be clear in summary.
• Read through code myself one more time.
• Make sure any and all TODO comments left behind are meant to be left in.
• Has reasonable passing test coverage?
• Updated changelog if applicable.
• Updated documentation if applicable.

[mondoreale]

@harbu @teogeb I've reformatted the files using 120 long print width. It definitely makes certain things look much better.

That it() formatted differently is a result of passing 3rd argument to it, i.e. it(label, fn, TIMEOUT). It then places the label in its own line. Pinned down one example of it. It'll be applied consistently to all similar scenarios if needed.

[mondoreale]

3rd argument (the timeout) forces Prettier to put the label onto its own line.

[harbu]

I wonder why this was put on a newline (but not the arguments for the callbacks below). Is it because of the async keyword?

[mondoreale]

Because of the presence of the 2nd and the 3rd arguments given to scheduleAtFixedRate.

scheduleAtFixedRate(
    async (now: number) => { /* code */ },
    interval,
    abortSignal
)

[harbu]

Not a fan of this comment enforcement. I think with comments there should be freedom to put them wherever to make it very clear what is reference.

[mondoreale]

It wouldn't touch a comment placed immediately after a statement, e.g.

if (firstSlashIdx === -1) {
    return streamIdOrPath as StreamID // something here about what just happened
}

To me placing a comment immediately after { feels a bit strange, generally, so I'm for this behaviour. FYI.

[harbu]

A lot going on in this statement. I sort of prefer the "before" here due to the complexity being broken down into lines.

[mondoreale]

Those long lines aren't ideal. Maybe smaller print width?

On the other hand... raceEvents3 could be organically async making it possible to restructure it like so:

export async function raceEvents3<T extends EventEmitter.ValidEventTypes>(
    emitter: EventEmitter<T>,
    eventNames: (keyof T)[],
    timeout: number | null = 5000
): Promise<RunAndRaceEventsReturnType<T>> {
    // …

    try {
        const taskRace = Promise.race(promises.map((promise) => promise.task))

        if (timeout !== null) {
            return await withTimeout(taskRace, timeout, 'raceEvents3')
        }

        return await taskRace
    } finally {
        promises.forEach((promise) => {
            promise.cancel()
        })
    }
}

So it's just possible if it was new code this formatting issue simply wouldn't occur.

[harbu]

Special case but here previous formatting is superior

[mondoreale]

Agreed.

Maybe if someone wrote it *now* (accompanied by a formatter) they'd do something like this instead:

describe('name', () => {
    function pad(str: string) {
        return typeof _streamr_electron_test === 'undefined' ? str.padEnd(25) : str
    }

    it('short', () => {
        expect(Logger.createName(module)).toBe(pad('Logger.test'))
    })

    it('application id', () => {
        process.env.STREAMR_APPLICATION_ID = 'APP'
        expect(Logger.createName(module)).toBe(pad('APP:Logger.test'))
        delete process.env.STREAMR_APPLICATION_ID
    })

    it('index', () => {
        expect(
            Logger.createName({
                id: ['foo', 'bar', 'mock', 'index'].join(path.sep)
            } as any)
        ).toBe(pad('mock'))
    })
})

[harbu]

Special case but here previous formatting is superior

[harbu]

Inconsistency with parameter list line break...

[harbu]

Imo too much going on in one line

[harbu]

Kind of like the earlier, ngl

[mondoreale]

I believe one can simply write

["src", "generated", "test"]

A matter of preference. I don't mind so I'd go with the flow.

[harbu]

I don't find this as redable..

[harbu]

I don't find this as readable

[mondoreale]

@harbu thanks for the commentary.

I agree with a lot of it. Having said that, I believe that with a consistent formatter in place most of what's there would've been written differently.

Left a couple of notes on your notes.

Cheers!

[mondoreale]

Because of the presence of the 2nd and the 3rd arguments given to scheduleAtFixedRate.

scheduleAtFixedRate(
    async (now: number) => { /* code */ },
    interval,
    abortSignal
)

[mondoreale]

It wouldn't touch a comment placed immediately after a statement, e.g.

if (firstSlashIdx === -1) {
    return streamIdOrPath as StreamID // something here about what just happened
}

To me placing a comment immediately after { feels a bit strange, generally, so I'm for this behaviour. FYI.

[mondoreale]

Those long lines aren't ideal. Maybe smaller print width?

On the other hand... raceEvents3 could be organically async making it possible to restructure it like so:

export async function raceEvents3<T extends EventEmitter.ValidEventTypes>(
    emitter: EventEmitter<T>,
    eventNames: (keyof T)[],
    timeout: number | null = 5000
): Promise<RunAndRaceEventsReturnType<T>> {
    // …

    try {
        const taskRace = Promise.race(promises.map((promise) => promise.task))

        if (timeout !== null) {
            return await withTimeout(taskRace, timeout, 'raceEvents3')
        }

        return await taskRace
    } finally {
        promises.forEach((promise) => {
            promise.cancel()
        })
    }
}

So it's just possible if it was new code this formatting issue simply wouldn't occur.

[mondoreale]

Agreed.

Maybe if someone wrote it *now* (accompanied by a formatter) they'd do something like this instead:

describe('name', () => {
    function pad(str: string) {
        return typeof _streamr_electron_test === 'undefined' ? str.padEnd(25) : str
    }

    it('short', () => {
        expect(Logger.createName(module)).toBe(pad('Logger.test'))
    })

    it('application id', () => {
        process.env.STREAMR_APPLICATION_ID = 'APP'
        expect(Logger.createName(module)).toBe(pad('APP:Logger.test'))
        delete process.env.STREAMR_APPLICATION_ID
    })

    it('index', () => {
        expect(
            Logger.createName({
                id: ['foo', 'bar', 'mock', 'index'].join(path.sep)
            } as any)
        ).toBe(pad('mock'))
    })
})

[mondoreale]

Yeah, that's the most annoying part. Prettier doesn't make sibling nodes consistent with each other.

No way around it.

[mondoreale]

Yep, old one reads better.

On the other hand, chaining tends to get ugly quickly, and very often is also a waste of processing effort. Here for example you don't need multiple passes. Consider this

const getValuesOfIncludedKeys = (
    nodes: Map<DhtAddress, ContentDeliveryRpcRemote>,
    exclude: DhtAddress[],
    wsOnly = false
): ContentDeliveryRpcRemote[] => {
    const remotes: ContentDeliveryRpcRemote[] = []

    for (const [id, node] of nodes.entries()) {
        if (wsOnly && node.getPeerDescriptor().websocket === undefined) {
            continue
        }

        if (exclude.includes(id)) {
            continue
        }

        remotes.push(node)
    }

    return remotes
}

Few benefits

• Single pass
• No extra array allocation (Array.from)
• Fairly clean
• Very little room for the formatter to mess up

Plus, to me, it feels much cleaner.

[mondoreale]

DRY-ing it up a little bit could help but yeah, I'm with you.

[mondoreale]

I'd throw a rough guess that the type casting doesn't help. 😉

[mondoreale]

I believe one can simply write

["src", "generated", "test"]

A matter of preference. I don't mind so I'd go with the flow.

[harbu]

So I did a bit of a readthru & analysis of the prettier formatting.

• I went thru all the code in packages utils and trackeless-network .
• I excluded changes related to white space, trailing commas, and interface/type semicolon (those can easily be enforced with any tool)
• For the remaining changes, I categorized and tallied them into the three categories based on readability
  - Good: change improved readability
  - Neutral: change neither improved or worsened readability
  - Bad: change worsened readability

So for the scores:
utils

Good 24 (23%)
Neutral 74 (70%)
Bad 7 (7%)

trackerless-network

Good 51 (19%)
Neutral 198 (74%)
Bad 19 (7%)