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

DRAFT: "AutoRouter" to reduce router initialization boilerplate #195

Closed
wants to merge 39 commits into from

Conversation

kwhitley
Copy link
Owner

@kwhitley kwhitley commented Nov 25, 2023

Description

Adds flow(router: Router, options?: FlowOptions): Response to reduce boilerplate. This function makes the following assumptions (with override controls):

  • Default response formatter as json
  • Includes default 404 handler on missed route
  • Includes default 500 error handler on uncaught errors
// BEFORE/WITHOUT FLOW
import { Router, json, error } from 'itty-router'

const router = Router()

router
  .get('/my-items', () => [1, 2, 3])
  .get('*', () => error(404))

export default {
  fetch: (...args) => router
                        .handle(...args)
                        .then(json)
                        .catch(error)
}
// AFTER/WITH FLOW
import { Router, flow } from 'itty-router'

const router = Router()

router.get('/my-items', () => [1, 2, 3])

export default {
  fetch: flow(router)
}

// or even

export default flow(router)
// WITH FLOW, OVERRIDES (including CORS)
import { Router, flow, error } from 'itty-router'

const router = Router()

router.get('/my-items', () => [1, 2, 3])

export default {
  fetch: flow(router, {
    cors: true,
    notFound: (req) => error(404, `The path "${req.url}" was not found.`),
  })
}

API (options)

Option Name Default Valid Choices
cors false false (off/ignore), true (default createCors()), CorsOptions
format json false (off/ignore), Function<Response>
errors error false (off/ignore), Function<Response>
notFound () => error(404) false (off/ignore), Function<Response>

Type of Change (select one and follow subtasks)

  • Documentation (README.md)
  • Maintenance or repo-level work (e.g. linting, build, tests, refactoring, etc.)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
    • I have included test coverage
  • Breaking change (fix or feature that would cause existing functionality/userland code to not work as expected)
    • Explain why a breaking change is necessary:
  • This change requires (or is) a documentation update
    • I have added necessary local documentation (if appropriate)
    • I have added necessary itty.dev documentation (if appropriate)

@kwhitley kwhitley added new feature New feature or request in progress I'm working on it... allegedly... PENDING RELEASE Final stages before release! :D Feedback Requested API Changes, interface questions, etc... labels Nov 25, 2023
kwhitley and others added 12 commits December 13, 2023 14:22
* Add the required `extends` calls to fix the ts-expect-error

* Get rid of the other typescript ignores

* Replace one  last expect-error with an `as`

In this case the 'as' is asserting "I know better than the compiler" and it
is both true and more accurate than saying "I expect typescript to error out
here." As an added benefit, `as` does more type checking than 'expect error',
so you get just a small bit more type safety.
Copy link

@Crisfole Crisfole left a comment

Choose a reason for hiding this comment

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

@kwhitley this is mostly an improvement, but you're missing more generics. That's what causes some 95% of your ts-ignore-error and any usages.

Pretty much every any you have can be replaced 1:1 with a generic type parameter for significantly better type signatures.

src/Router.ts Outdated
export type RouteHandler<I = IRequest, A extends any[] = any[]> = {
(request: I, ...args: A): any
export type RouteHandler<R = IRequest, Args = any[]> = {
// @ts-expect-error - TS never likes this syntax

Choose a reason for hiding this comment

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

Doesn’t like the syntax because you need [] array type for spread args.

Choose a reason for hiding this comment

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

You’d need an extends Array in your type handler here, I think

export interface CorsFns {
corsify(req: Request): Response;
preflight(r: IRequest): Response;
export interface CorsHandlers {

Choose a reason for hiding this comment

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

MUCH better name. Thanks for the fix.

preflight(r: IRequest): Response;
export interface CorsHandlers {
corsify(response: Response): any
preflight(Request: IRequest): any

Choose a reason for hiding this comment

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

Never use a direct IRequest anywhere but as a default for a generic type. It'd be best if you setup a linter for this, tbh. It's the source of 99% of your type bugs.

@@ -38,20 +38,20 @@ export const createCors = (options: CorsOptions = {}): CorsFns => {
if (maxAge) rHeaders['Access-Control-Max-Age'] = maxAge

// Pre-flight function.
const preflight = (r: IRequest) => {
const preflight = (request: IRequest): any => {

Choose a reason for hiding this comment

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

createCors needs to be generic to get the types to behave.

@@ -20,7 +20,7 @@ export type FlowOptions = {
after?: afterFunction
}

export type Flowed = (request: RequestLike, ...extra: any[]) => Promise<any>
export type Flowed = (request: IRequest, ...extra: any[]) => Promise<any>

Choose a reason for hiding this comment

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

This has to be generic to get the handler types working nicely.

@@ -20,7 +20,7 @@ export type FlowOptions = {
after?: afterFunction
}

export type Flowed = (request: RequestLike, ...extra: any[]) => Promise<any>
export type Flowed = (request: IRequest, ...extra: any[]) => Promise<any>
export type FlowedAndFetch = Flowed & { fetch: Flowed }

Choose a reason for hiding this comment

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

This is a real weird type. It's a fetch function that owns a ... fetch function?

@kwhitley
Copy link
Owner Author

Adding to the discussion here... flow has been shelved for an arguably better router approach. Notes to follow.

@kwhitley kwhitley changed the title New Feature: "flow" to reduce router initialization boilerplate New Feature: "AutoRouter" to reduce router initialization boilerplate Jan 8, 2024
@kwhitley kwhitley changed the title New Feature: "AutoRouter" to reduce router initialization boilerplate DRAFT: "AutoRouter" to reduce router initialization boilerplate Jan 8, 2024
@kwhitley
Copy link
Owner Author

Killed in favor of #222

@kwhitley kwhitley closed this Mar 15, 2024
@kwhitley kwhitley added ON HOLD and removed in progress I'm working on it... allegedly... PENDING RELEASE Final stages before release! :D new feature New feature or request Feedback Requested API Changes, interface questions, etc... labels Mar 20, 2024
@kwhitley kwhitley deleted the flow branch March 24, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants