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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add semi-functional Svelte router 馃帀 #9

Merged
merged 9 commits into from
Jun 20, 2022
Merged

Conversation

austincrim
Copy link
Collaborator

Adds scaffolding and WIP implementation for remix-router-svelte

Known TODOS:

  • Fix useLoaderData reactivity bug
  • Figure out solution for useFetcher form
  • Configure build/publish setup
  • Find and squash inevitable bugs
  • Documentation, I suppose

supportFile: false,
video: false,
env: {
UI_LIBRARY: library,
Copy link
Owner

Choose a reason for hiding this comment

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

Added this to use Cypress.env('UI_LIBRARY') to conditionally skip certain test suites

const { conditionalDescribe } = require("../utils/utils");

conditionalDescribe(
{ react: true, vue: true, svelte: false },
Copy link
Owner

Choose a reason for hiding this comment

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

Skip the error_spec files for svelte

@@ -0,0 +1,7 @@
Copyright 2022 Matt Brophy <matt@brophy.org>
Copy link
Owner

Choose a reason for hiding this comment

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

Don't forget to change this over to yourself @austincrim!

@@ -0,0 +1,104 @@
# remix-router-vue
Copy link
Owner

Choose a reason for hiding this comment

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

Before publishing let's get this updated accordingly 馃憤

@@ -0,0 +1,55 @@
{
"name": "remix-router-svelte",
"version": "0.3.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"version": "0.3.0",
"version": "0.1.0",


<h3>Child Route</h3>

<!-- currently broken! -->
Copy link
Owner

Choose a reason for hiding this comment

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

Not anymore, right??

/* eslint-disable @typescript-eslint/no-explicit-any */
/* eslint-disable @typescript-eslint/no-unsafe-assignment */
/* eslint-disable @typescript-eslint/no-unsafe-member-access */
import type { FormEncType, FormMethod } from "@remix-run/router";
Copy link
Owner

Choose a reason for hiding this comment

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

Just FYI - eventually this will all come from @remix-run/router, it's just not exported at the moment. So I would avoid making any changes in here as you keep moving on svelte. If you find bugs or need to change anything - we'll want to note that and make the corresponding change to the file currently in react-router-dom. Hopefully we get the exports reworked in the 6.4 release in the next couple weeks though.


export function useLocation(): Readable<Location> {
let ctx = getRouterContext();
return derived(ctx.state, ({ location }) => location);
Copy link
Owner

Choose a reason for hiding this comment

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

Any idea if these suffer the same issue that useLoaderData did using the derived store instead of getStoreSnapshot?

external: ["@remix-run/router", "svelte"],
output: {
globals: {
"@remix-run/router": "Router",
Copy link
Owner

Choose a reason for hiding this comment

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

Do you need "svelte": "Svelte" here too?

Copy link
Owner

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

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

This looks great! I pushed up a commit to allow conditional testing to cypress suites and disabled error_spec for svelte. Feel free to merge this and then keep working against that branch in new PRs.

I'm going to start looking at adding changesets in main - so as soon as you're comfortable with the base functionality and have yarn build and package.json files all configured we should be good to publish a 0.1.0 馃憤

"prepublishOnly": "npm run build"
},
"dependencies": {
"@remix-run/router": "0.1.0",
Copy link
Owner

Choose a reason for hiding this comment

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

I've updated main to the latest version here so you'll want to update this to match what's in react/vue - currently 0.2.0-pre.2

@austincrim
Copy link
Collaborator Author

Thanks for the review! I'm going to merge and then address your comments in a subsequent PR 馃憤.

@austincrim austincrim merged commit 8a2e1c6 into svelte Jun 20, 2022
@austincrim austincrim deleted the remix-router-svelte branch June 20, 2022 18:02
austincrim added a commit that referenced this pull request Jul 5, 2022
* address pr #9 comments, useLoaderData and useFetcher actually working

* pr comments
austincrim added a commit that referenced this pull request Aug 5, 2022
* Add semi-functional Svelte router 馃帀  (#9)

* create svelte package, functional nested routing!

* <Link>, loaders functional

* Forms, fetchers functional

* more router hooks

* folder structure

* replace accidental edits

* fix??? useLoaderData

* Add conditional testing to cypress

* Update error message

Co-authored-by: Matt Brophy <matt@brophy.org>

* address pr #9 comments, fix useLoaderData and useFetcher (#11)

* address pr #9 comments, useLoaderData and useFetcher actually working

* pr comments

* get new e2e tests passing

* Configure build for `remix-router-svelte` (#13)

* reorg files, use svelte-kit package

* typo

* cleanup before publish

* cleanup before publish

* "fix" reference-app ts errors

* more cleanup

* fix cypress config file name

* fix tests?

Co-authored-by: Matt Brophy <matt@brophy.org>
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.

None yet

2 participants