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: move to sveltekit #71

Merged
merged 10 commits into from
Nov 5, 2023
Merged

feat: move to sveltekit #71

merged 10 commits into from
Nov 5, 2023

Conversation

jamesdabbs
Copy link
Member

@jamesdabbs jamesdabbs commented Aug 27, 2023

Replacing svelte-router based routing. This commit makes a minimal set of changes required to get things functional. We'll follow up with other changes to move things around, make them more idiomatically sveltekit, and start leveraging some of its features a little better.

Fixes #65

@jamesdabbs jamesdabbs self-assigned this Aug 27, 2023
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 27, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: ccc703d
Status: ✅  Deploy successful!
Preview URL: https://f6f02133.topology.pages.dev
Branch Preview URL: https://sveltekit.topology.pages.dev

View logs

Replacing svelte-router based routing. This commit makes a minimal
set of changes required to get things functional. We'll follow up
with other changes to move things around, make them more
idiomatically sveltekit, and start leveraging some of its features
a little better.
@jamesdabbs
Copy link
Member Author

@StevenClontz – I'd love to get your thoughts here. I will fix up the Cypress tests, but I think that'll be easier to do after doing some more substantial reworking of the app booting and data loading process. I'm weakly inclined to break that up into several different PRs into the main branch, but wanted to get your take. I'm not expecting to merge any PRs that knowingly break things, but there may be a few small regressions around the edges (browser / server differences, local storage, &c.) that get fixed up later along with the end-to-end tests.

@StevenClontz
Copy link
Member

StevenClontz commented Aug 27, 2023

Things that probably don't surprise you based on my messing with the public preview: changing branches updates the data but doesn't re-run deductions. Then refreshing loses the choice of an alternate branch, and doesn't re-run deductions until I hit reset. (EDIT: I'll try to remember to use this preview next time I want to look up things on main, to see if there's any breakage when not messing with branches.)

Unless I can pitch in with the refactor directly, I defer to you on how you want to proceed with it, and I'm happy to keep trying to use preview deploys for reviewing/contributing to pi-base/data. But if you want to collaborate on any pi-base/web development, I probably want to sync up and may have a stronger opinion on the workflow.

In cases where the sync completes immediately, the unsubscribe
callback isn't fully initialized. We were checking for this
logically, but the const declaration prevents access. var is more
permissive and defaults to undefined, so the && check works as
intended.
@jamesdabbs jamesdabbs force-pushed the sveltekit branch 10 times, most recently from be0df25 to da862f2 Compare November 5, 2023 02:05
Moving to sveltekit complicates trying to intercept network calls,
as they can happen either on the client or server side. The approach
we're taking here is to serve the fixture data from the local preview
server during testing.
@jamesdabbs jamesdabbs merged commit 6a6329b into main Nov 5, 2023
3 checks passed
@jamesdabbs jamesdabbs deleted the sveltekit branch November 5, 2023 03:23
@StevenClontz
Copy link
Member

Thanks for your progress on this James. I'll have the community take a peek (unless there's a reason to wait)

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.

Brittle branch changing
2 participants