search

ipfs / helia

An implementation of IPFS in JavaScript
https://helia.io
Other
812 stars 81 forks source link

fix: unnecessary new URL instance #502

Closed SgtPooki closed 2 months ago

SgtPooki commented 2 months ago

https://github.com/ipfs/helia/blob/13daa26ec9bc6c1a1552b7b56597945c5b13a87b/packages/block-brokers/src/trustless-gateway/trustless-gateway.ts#L52

this.url in TrustlessGateway instances is already a URL (in constructor) and should not need to use const gwUrl = new URL(this.url.toString())

SgtPooki commented 2 months ago

nvm. This is to prevent modifying the readonly this.url, but we could likely set gwURL in the constructor, or modify the url there

SgtPooki commented 2 months ago

we could also construct a request instance and use that in the fetch call. micro-optimization, but still

achingbrain commented 2 months ago

This is to prevent modifying the readonly this.url

Exactly, yes.

we could likely set gwURL in the constructor

We can't, because getRawBlock is called multiple times, and that function modifies the URL object, so it needs to make a copy of it, to avoid modifying the internal state of the broker URL.

micro-optimization, but still

If you can measure it, and make a real-world performance improvement it's worth doing but I'm not sure in this case.

Closing as this isn't a bug.