Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove unused helia, use verified fetch, and simplify code #89

Merged
merged 10 commits into from
Mar 9, 2024

Conversation

2color
Copy link
Member

@2color 2color commented Mar 6, 2024

PR Summary

This PR is mostly refactoring with some small changes to make it easier to implement caching.

  • Replace the singleton helia instance in favour of a verifiedFetch instance because the helia instance wasn't actually used
  • Remove the indexedDB block store - this is a temporary measure until I decide how to implement the cache in Cache final Responses #73. Since the helia instance wasn't used and verifiedFetch was instantiated on every request, this has no effect
  • Simplify the heliaFetch function and break it into more easily testable functions
  • Move the default routers and gateways config to storage so that the user can also customise the default config (not finished since it's initially loaded from localStorage which can probably removed as per discussion)
  • Fixes Slow iframe requests to root domain (unintended cache bypass) #88 by loading the config page using a hash fragment instead of query param

Notes & open questions

  • We may still want to use indexedDB blockstore for caching (related Cache final Responses #73) . If so, see fix!: move dnsResolvers to options helia-verified-fetch#13 which should enable us to use it while customising the DoH endpoints.
  • Config passing between origins needs to be more carefully tested. It seems like in some cases this is not working properly, e.g. when auto-reload is set up and there isn't enough time for the message to be passed from the root to the subdomain origin.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@2color 2color requested review from lidel and SgtPooki March 6, 2024 14:02
@2color 2color changed the title refactor: remove unused helia and use verified fetch refactor: remove unused helia, use verified fetch, and simplify code Mar 6, 2024
@2color 2color mentioned this pull request Mar 7, 2024
3 tasks
@SgtPooki
Copy link
Member

SgtPooki commented Mar 8, 2024

A few comments before I review:

Replace the singleton helia instance in favour of a verifiedFetch instance because the helia instance wasn't actually used

👍 great, yea this needed done!

Move the default routers and gateways config to storage so that the user can also customise the default config (not finished since it's initially loaded from localStorage which can probably removed #86 (comment))

moved them to which storage if we removed indexedDB? nvm, i see that they're moved to localStorage.

Move the default routers and gateways config to storage so that the user can also customise the default config (not finished since it's initially loaded from localStorage which can probably removed #86 (comment))

I believe in the last helia WG call we discussed not removing localStorage.

We may still want to use indexedDB blockstore for caching (related #73) . If so, see ipfs/helia-verified-fetch#13 which should enable us to use it while customising the DoH endpoints.

We do still want to use indexedDB for passing data to and from the config page. I don't think we need the indexedDB blockstore at all. A plain indexedDB instance should work

Config passing between origins needs to be more carefully tested. It seems like in some cases this is not working properly, e.g. when auto-reload is set up and there isn't enough time for the message to be passed from the root to the subdomain origin.

There is a TODO to use the commsChannel to use passMessageAndWaitForResponse (or whatever its called) which would require waiting until the message is passed and sw responds back

Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of these changes in general. I think there are a few changes we should get in before merging though.

Thanks Daniel!

src/components/config.tsx Outdated Show resolved Hide resolved
src/get-helia.ts Show resolved Hide resolved
src/lib/heliaFetch.ts Outdated Show resolved Hide resolved
src/sw.ts Outdated Show resolved Hide resolved
@@ -97,6 +103,7 @@ const fetchHandler = async ({ path, request }: FetchHandlerArg): Promise<Respons
return new Response('heliaFetch error: ' + errorMessage, { status: 500 })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return new Response('heliaFetch error: ' + errorMessage, { status: 500 })
return new Response('verifiedFetch error: ' + errorMessage, { status: 500 })

src/sw.ts Outdated Show resolved Hide resolved
* fix: config loading on subdomains waits for updated config

* fix: remove event.waitUtil inside channel.onmessagefrom

the activate event is fired when the service worker is first activated,
and then the channel.onmessagefrom event listener is set up.
by the time the channel receives any messages, the event passed to activate
will be done, and waitUntil has no effect (and throws an error)

* fix: service worker handles being disposed of

* fix: preload request from redirect page is handled by sw

* chore: better trace logging for config updates
@SgtPooki SgtPooki merged commit 0a6d2f3 into main Mar 9, 2024
18 checks passed
@SgtPooki SgtPooki deleted the refactor-for-cache branch March 9, 2024 00:31
@lidel lidel mentioned this pull request Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow iframe requests to root domain (unintended cache bypass)
2 participants