Skip to content

Conversation

@jycouet
Copy link
Owner

@jycouet jycouet commented Nov 26, 2023

It's WIP

Todo

Breaking

  • methods & actions are not the first param anymore. They are part of the key
export const SERVERS = {
  GET_contract: (params?: { lang?: 'fr' | 'hu' }) => {
    return `${params?.lang ? `/${params?.lang}` : ''}/contract`
  },
  POST_contract: (params?: { lang?: 'fr' | 'hu' }) => {
    return `${params?.lang ? `/${params?.lang}` : ''}/contract`
  },
  GET_api_graphql: `/api/graphql`,
  POST_api_graphql: `/api/graphql`,
}
export const ACTIONS = {
  default_contract_id: (params: {
    id: string | number
    lang?: 'fr' | 'at'
    limit?: number
  }) => {
    return `${params?.lang ? `/${params?.lang}` : ''}/contract/${params.id}${appendSp({
      limit: params?.limit,
    })}`
  },
  update_site_id: (params: { id: string, lang?: 'fr' | 'hu' }) => {
    return `${params?.lang ? `/${params?.lang}` : ''}/site?/update`
  },
  delete_site_id: (params: { id: string, lang?: 'fr' | 'hu' }) => {
    return `${params?.lang ? `/${params?.lang}` : ''}/site?/delete`
  },
}

Question A/
I'm not sure about prepending and appending yet... I like PAGES.update_site_id({ id: 7 }), it's very "self explain".
Should prepend / append be an option or I leave it like this?

  • Optional params are not part of the key anymore
    I learned that it cannot do collision (SvelteKit will complain if not)

  • New format / New default?

// A format "/" the default today
PAGES["/site/[id]"]({ id: 7 })
// B a new option?
route("/site/[id]", { id: 7 })
// C format "_"
PAGES.site_id({ id: 7 })
// D a new option?
route("site_id", { id: 7})
// E format "variable"
PAGES_site_id({ id: 7 })

Question B/
In format "variable", should we prefix by PAGES_ / ACTIONS_ / SERVERS_ / LINKS_ (like today) or ROUTE_ or route_ or nothing?

Question C/
Should it be options?


I tag you @tmarnet @xKesvaL @hmnd @brandonp-ais as you tried and reported already something 🎉
If you have a few input, I would be happy to check :)
(code is not working yet.)

@vercel
Copy link

vercel bot commented Nov 26, 2023

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

Name Status Preview Comments Updated (UTC)
kitql ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 9:15pm

@xKesvaL
Copy link

xKesvaL commented Nov 26, 2023

I like format c the most (PAGES.site_id({ id: 7 }) tbh

@tmarnet
Copy link

tmarnet commented Nov 26, 2023

Out of all formats, option B feels the most intuitive to me (route("/site/[id]", { id: 7 })), especially with TypeScript autocompleting the paths.

Comment on lines 69 to 88
const ACTIONS = {
"default /contract/[id]": (params: { id: (string | number), lang?: ('fr' | 'en' | 'hu' | 'at' | string) }) => {
return `${params?.lang ? `/${params?.lang}`: ''}/contract/${params.id}`
},
"create /site": (params?: { lang?: ('fr' | 'en' | 'hu' | 'at' | string) }) => {
return `${params?.lang ? `/${params?.lang}`: ''}/site?/create`
},
"update /site/[id]": (params: { id: (string | number), lang?: ('fr' | 'en' | 'hu' | 'at' | string) }) => {
return `${params?.lang ? `/${params?.lang}`: ''}/site/${params.id}?/update`
},
"delete /site/[id]": (params: { id: (string | number), lang?: ('fr' | 'en' | 'hu' | 'at' | string) }) => {
return `${params?.lang ? `/${params?.lang}`: ''}/site/${params.id}?/delete`
},
"noSatisfies /site_contract": (params?: { lang?: ('fr' | 'en' | 'hu' | 'at' | string) }) => {
return `${params?.lang ? `/${params?.lang}`: ''}/site_contract?/noSatisfies`
},
"send /site_contract/[siteId]-[contractId]": (params: { siteId: (string | number), contractId: (string | number), lang?: ('fr' | 'en' | 'hu' | 'at' | string) }) => {
return `${params?.lang ? `/${params?.lang}`: ''}/site_contract/${params.siteId}-${params.contractId}?/send`
}
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

action /the/path, and a default action is named also

Copy link

Choose a reason for hiding this comment

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

Maybe we could skip the default prefix for convenience?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Unfortunately, with the function route() we can have collision quite fast. (Here with a page for example.

So, maybe I'll have to keep it. I don't really like it.
Or maybe having the default only if collision?

Copy link

@tmarnet tmarnet Nov 27, 2023

Choose a reason for hiding this comment

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

You mean the route() function, in its current implementation, would be the single entry point for everything (pages, actions, servers, etc.) right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

yes 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

(OMG, experience of comments on a PR is a disaster, I get things in wrong order... And I'm not even sure I saw everything)

Copy link

@tmarnet tmarnet Nov 27, 2023

Choose a reason for hiding this comment

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

Would there be any benefit of having separate functions (just like we have separate objects, could be page(), action(), etc.) rather than a single route() function?

I can already see 2:

  • It'd fix the name collision issue
  • It'd also make it clear if you're pointing to a page, an action, etc. and avoid mistakes

Any drawbacks that you can think of?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think that collision can happen only* with the default (I could almost rename it action instead of default? )

The highest request I had was to merge everything into one object ROUTES. Then we came into the fonction 'route()' where people wanted everything.

So... I don't know... I think it's already a lot of options 😅

(and I probably want to provide one option when there is only 1 required param to give it without object!) 😳

You are somewhere to chat live? 😊

Copy link

Choose a reason for hiding this comment

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

I think that collision can happen only* with the default

Yes

I could almost rename it action instead of default?

Let's keep SvelteKit's semantics and use default

You are somewhere to chat live?

Just sent you a Discord invite 🙂

@jycouet jycouet marked this pull request as ready for review November 27, 2023 20:33
@jycouet jycouet closed this Nov 27, 2023
@jycouet jycouet reopened this Nov 27, 2023
@jycouet
Copy link
Owner Author

jycouet commented Nov 27, 2023

let's do a .next tag 🎉

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.

4 participants