evidence-dev / evidence

Business intelligence as code: build fast, interactive data visualizations in pure SQL and markdown
https://evidence.dev
MIT License
3.44k stars 167 forks source link

540 upgrade to sveltekit 1.0 #628

Closed mcrascal closed 1 year ago

mcrascal commented 1 year ago

Description

Draft PR for the svelte kit 1.0 upgrade

There are still some blocking, and some more minor issues here.

Blocking:

CLI File Watcher

New Node Requirements

Other

Minor:

changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: d489c66565d0e63898003cc994700f60105a7942

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages | Name | Type | | ----------------------------- | ----- | | @evidence-dev/db-orchestrator | Major | | @evidence-dev/evidence | Major | | @evidence-dev/preprocess | Major | | evidence-docs | Major | | @evidence-dev/components | Major | | evidence-test-environment | Major |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
evidence-development-workspace ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 1, 2023 at 6:42PM (UTC)
evidence-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 1, 2023 at 6:42PM (UTC)
Winterhart commented 1 year ago

I took an early look 👀 at this PR today. It's far from main we should catch up with main (rebase or merge)

Winterhart commented 1 year ago

Error: [vite]: Rollup failed to resolve import "svelte-tiny-linked-charts" from "/home/runner/work/evidence/evidence/packages/components/viz/BigValue.svelte".

In my branch, I've moved up the svelte-tiny-linked-charts the be a workspace dependency

Winterhart commented 1 year ago

Also (my theory) since the branch is coming from a fork we can't edit the .github/workflows folder we might have to merge with failing node-14

mcrascal commented 1 year ago

@Winterhart I think I've gotten to the bottom of the flaky behaviour on parameterized pages. Just taking my notes here.

  1. There is a race condition between query extraction, and the load function.
  2. Query extraction happens as a side effect of our markdown preprocessor. This hasn't changed (yet).
  3. Previously, load functions were added to pages' source as part of the preprocessor as well. In this update, they are moved into a proper server load function. This is a big improvement, but it means that during the first visit to a page our load function can return before query extraction has completed.
  4. I have moved query extraction out of preprocess, and into an endpoint

There are some remaining issues:

Winterhart commented 1 year ago

This is a quite tough PR. I will try it locally today.

mcrascal commented 1 year ago

@Winterhart, @hughess:

This is ready for review. A couple things:

  1. This removes support for node 14 (those tests will always fail)
  2. Because we've moved some deps around between preprocess and the internal APIs, I suspect we'll find some failures when we release into a next branch
hughess commented 1 year ago

Everything seems to be working well when I test in my browser, but I've been getting a stream of error messages in my dev console. It's these two error messages rotating back and forth:

Error: Not found: /api/6666cd76f96956469e7be39d750cc7d9/status.json Error: Not found: /__vite_ping

hughess commented 1 year ago

Nevermind, this was my own error 🤦 - I left a browser tab open to a page that wasn't in the project so it was complaining. I had too many tabs open!

hughess commented 1 year ago

I see the pages in example-project have been converted over to the new +page version. I was hoping to test that our current way of creating pages still works (individual .md file, index.md inside a folder, other .md inside a folder with or without an index.md file).

Is test-env the right place for that testing?

mcrascal commented 1 year ago

I see the pages in example-project have been converted over to the new +page version. I was hoping to test that our current way of creating pages still works (individual .md file, index.md inside a folder, other .md inside a folder with or without an index.md file).

Is test-env the right place for that testing?

Yes

mcrascal commented 1 year ago

@hughess -- good shout on the file structure stuff, I see two cases that aren't working quite right:

hughess commented 1 year ago

Ok I tested the various page scenarios and all worked except for one - an index page inside a folder which contains other pages.

In this example I have index.md and second-in-folder.md inside the folder directory. The index page isn't getting picked up as the href for the breadcrumbs or the sidebar:

CleanShot 2023-02-16 at 15 05 09

If I navigate to /folder by typing in the URL, the page works as expected.