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

feat: create config page for sw settings #24

Merged
merged 29 commits into from
Feb 27, 2024
Merged

feat: create config page for sw settings #24

merged 29 commits into from
Feb 27, 2024

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Feb 21, 2024

partially addresses #22

Todo

Preview Give feedback

Copy link
Member Author

@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.

quick review. still in draft cause it can use a bit more cleanup

src/sw.ts Outdated Show resolved Hide resolved
src/lib/channel.ts Outdated Show resolved Hide resolved
src/lib/channel.ts Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
Comment on lines +9 to +10
if (isConfigExpanded) {
return (<Config />)
Copy link
Member Author

Choose a reason for hiding this comment

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

eventually, this landing page should only be the config page....?

@SgtPooki SgtPooki self-assigned this Feb 21, 2024
@SgtPooki

This comment was marked as resolved.

@SgtPooki SgtPooki marked this pull request as ready for review February 22, 2024 23:52
@SgtPooki SgtPooki requested review from a team and lidel February 22, 2024 23:52
@SgtPooki

This comment was marked as resolved.

@SgtPooki

This comment was marked as resolved.

@SgtPooki
Copy link
Member Author

SgtPooki commented Feb 23, 2024

Okay, so I found out that if you empty the config (gateways and routers to "[]"), it works fine; I will investigate further.

I think ff7bafe fixed this.

@SgtPooki
Copy link
Member Author

question here: do we want the user to be able to override all of the gateways & routers? currently the config.gateways and routers are appended:

    blockBrokers: [
      trustlessGateway({
        gateways: ['https://trustless-gateway.link', ...config.gateways]
      })
    ],
    routers: ['https://delegated-ipfs.dev', ...config.routers].map(rUrl => delegatedHTTPRouting(rUrl))

@aschmahmann
Copy link
Contributor

IMO getting shown a status page with a lot of options on initial load is a little unfriendly. Maybe just a "hey welcome to the service worker gateway, if you want to change settings or for more information go to "

@aschmahmann
Copy link
Contributor

do we want the user to be able to override all of the gateways & routers? currently the config.gateways and routers are appended:

Other opinions welcome I'd probably start with append, and then we can figure out removal. Right now it's a bit tempting to just show the list of URLs to modify, but we may have some routers that are more complicated (e.g. a validating ENS router, or if IPNI federation gets off the ground)

@SgtPooki
Copy link
Member Author

IMO getting shown a status page with a lot of options on initial load is a little unfriendly. Maybe just a "hey welcome to the service worker gateway, if you want to change settings or for more information go to "

I can leave it collapsed by default and have autoReload default to true?

@SgtPooki
Copy link
Member Author

also see #19 (comment)

@SgtPooki
Copy link
Member Author

SgtPooki commented Feb 26, 2024

chatted with lidel

  • config page is useful for educating users, we should not auto-reload. require user interaction for reloading, unless they enable autoreload
  • Link to docs for gateways
  • link to docs for routers

@SgtPooki
Copy link
Member Author

new default page view:

Initial

image

After clicking "view config"

image

const [isCollapsed, setCollapsed] = useState(collapsed)

return (
<React.Fragment>
Copy link
Member Author

Choose a reason for hiding this comment

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

vscode was being weird in this file yelling about all types of syntax errors that didn't exist after closing it and re-opening. ¯\_(ツ)_/¯

Comment on lines +13 to +21
const buttonClasses = new Set(['button-reset', 'pv3', 'tc', 'bn', 'white', 'w-100', 'cursor-disabled', 'bg-gray'])
if (isServiceWorkerRegistered) {
buttonClasses.delete('bg-gray')
buttonClasses.delete('cursor-disabled')
buttonClasses.add('bg-animate')
buttonClasses.add('bg-black-80')
buttonClasses.add('hover-bg-aqua')
buttonClasses.add('pointer')
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this should probably be a useMemo and two distinct string values

Comment on lines +16 to +24
root.render(
<React.StrictMode>
<ServiceWorkerProvider>
<ConfigProvider expanded={window.location.pathname === '/config'}>
<App />
</ConfigProvider>
</ServiceWorkerProvider>
</React.StrictMode>
)
Copy link
Member Author

Choose a reason for hiding this comment

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

simplified root logic. page rendering determined in app.tsx

* Intended only for pushing from SW to the UI output terminal
*/
SHOW_STATUS = 'SHOW_STATUS',
RELOAD_CONFIG = 'RELOAD_CONFIG',
Copy link
Member Author

Choose a reason for hiding this comment

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

we probably need a RELOAD_CONFIG_DONE here that SW emits so we can block on config saving until the SW is refreshed with any updated config. This should prevent any confusion when adding a new gateway and not seeing requests go to that gateway.


export interface ConfigDb {
gateways: string[]
routers: string[]
autoReload: boolean
debug: string
Copy link
Member Author

Choose a reason for hiding this comment

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

you can use https://npmjs.com/package/debug strings to set debug levels on the service worker. similar to setting DEBUG=... env var in the terminal

export interface UrlParts {
id: string | null
protocol: string | null
parentDomain: string
Copy link
Member Author

Choose a reason for hiding this comment

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

moved this over from sw mostly so i could use this for iframe rendering of the config page now that we dont have BASE_URL. Only change is the addition of this parentDomain

channel.postMessage({ target: 'SW', action: 'RELOAD_CONFIG' })
// try to preload the content
setTimeout(() => {
fetch(window.location.href, { method: 'GET' }).then((response) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

when user is done on config page, "load content" or refresh should result in immediate rendering (or at least less wait time)

@SgtPooki
Copy link
Member Author

will merge this so we can iterate on any changes people want. lots of cleanup done in this PR

@SgtPooki SgtPooki merged commit d933208 into main Feb 27, 2024
12 checks passed
@SgtPooki SgtPooki deleted the feat/config-page branch February 27, 2024 17:16
@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.

3 participants