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(ssg): enhance conbined hooks #2686

Merged
merged 4 commits into from
May 24, 2024
Merged

Conversation

watany-dev
Copy link
Contributor

@watany-dev watany-dev commented May 16, 2024

We are considering making it easier for users to specify hooks without having to be aware of their placement by ultimately passing a list of each hook to either plugins or hooks. As a preliminary step, it is essential to enable each hook to combine multiple hooks. Without this capability, implementing such a patch for extension would be challenging.

Combining Hook

You can enhance your site generation process by combining multiple hooks.
While this approach can be applied to any type of hook (BeforeRequestHook, AfterResponseHook, AfterGenerateHook), the example below demonstrates combining multiple BeforeRequestHook instances to achieve fine-grained request filtering.

Example:

Combining BeforeRequestHook. First, define two hooks: one to allow requests for certain paths, and another to deny requests for specific paths.

// Hook to allow requests only to specified paths
const filterPathsBeforeRequestHook = (allowedPaths: string | string[]): BeforeRequestHook => {
  const baseURL = 'http://localhost';
  return async (req: Request): Promise<Request | false> => {
    const paths = Array.isArray(allowedPaths) ? allowedPaths : [allowedPaths];
    const pathname = new URL(req.url, baseURL).pathname;
    return paths.some(path => pathname === path || pathname.startsWith(`${path}/`)) ? req : false;
  };
};

// Hook to deny requests to specified paths
const denyPathsBeforeRequestHook = (deniedPaths: string | string[]): BeforeRequestHook => {
  const baseURL = 'http://localhost';
  return async (req: Request): Promise<Request | false> => {
    const paths = Array.isArray(deniedPaths) ? deniedPaths : [deniedPaths];
    const pathname = new URL(req.url, baseURL).pathname;
    return !paths.some(path => pathname === path || pathname.startsWith(`${path}/`)) ? req : false;
  };
};

Combine these hooks to filter requests by allowing them for /public path, but denying them for /public/secret.

toSSG(app, fs, {
  beforeRequestHook: [
    filterPathsBeforeRequestHook(['/public']),  // Allow only '/public'
    denyPathsBeforeRequestHook(['/public/secret'])  // Deny '/public/secret'
  ],
})

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun denoify to generate files for Deno
  • bun run format:fix && bun run lint:fix to format the code

@yusukebe
Copy link
Member

Hi @watany-dev

Sorry to be late for my comment. I think this is good! But I want to know, is the main point of this PR that makes the hooks allow receiving the array?

export interface ToSSGOptions {
  dir?: string
  beforeRequestHook?: BeforeRequestHook | BeforeRequestHook[] // <---
  afterResponseHook?: AfterResponseHook | AfterResponseHook[] // <---
  afterGenerateHook?: AfterGenerateHook | AfterGenerateHook[] // <---
  concurrency?: number
  extensionMap?: Record<string, string>
}

I'd like to know the options of others. cc: @nakasyou @usualoma @sor4chi

@yusukebe yusukebe added the v4.4 label May 22, 2024
@usualoma
Copy link
Member

Nice feature!
This is the natural behavior expected by users and I hope it is supported.

@yusukebe yusukebe changed the base branch from main to next May 22, 2024 20:37
Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

Hey @watany-dev

I think there is no problem. It can be merged!

@yusukebe yusukebe merged commit 04caa07 into honojs:next May 24, 2024
10 checks passed
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.

3 participants