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(context): Introduce ContextVariableMap. #429

Merged
merged 2 commits into from
Jul 30, 2022

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented Jul 28, 2022

Allows the user to type the value of ctx.set('variable', var) or ctx.get('variable').

This is one of the ideas to solve the #414 problem, but even if this were not used to solve #414, I think this would still be a useful feature.

@usualoma usualoma force-pushed the feat-context-variable-map branch from c30b87f to e46c2c1 Compare July 28, 2022 21:56
@usualoma usualoma force-pushed the feat-context-variable-map branch from e46c2c1 to a5d4d3b Compare July 28, 2022 21:59
@usualoma usualoma mentioned this pull request Jul 28, 2022
@@ -7,6 +7,8 @@ import { METHOD_NAME_ALL, METHOD_NAME_ALL_LOWERCASE } from './router'
import { TrieRouter } from './router/trie-router' // Default Router
import { getPathFromURL, mergePath } from './utils/url'

export interface ContextVariableMap {}

Choose a reason for hiding this comment

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

I forget the specifics, but this should probably extend Record<string, any>

Copy link
Member Author

Choose a reason for hiding this comment

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

In this PR, there is no need to extend Record<string, any> since ContextVariableMap is a type for type definition and no object is created. (This is like HTMLElementTagNameMap.)
Also, not extending Record<string, any> will give better completion results.

src/context.ts Outdated
@@ -20,7 +20,10 @@ export interface Context<RequestParamKeyType extends string = string, E = Env> {
header: (name: string, value: string) => void
status: (status: StatusCode) => void
set: (key: string, value: any) => void

Choose a reason for hiding this comment

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

Set should probably also attempt to enforce the type of value if it's specified on ContextVariableMap for extra type safety.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!
Fixed in 1852968.

@usualoma usualoma changed the title feat(context): Introduce HonoContextVariableMap. feat(context): Introduce ContextVariableMap. Jul 29, 2022
@usualoma usualoma marked this pull request as ready for review July 29, 2022 10:17
@yusukebe
Copy link
Member

Thank you! Looks good!

@yusukebe yusukebe merged commit bdcd536 into honojs:main Jul 30, 2022
@yusukebe yusukebe added the add label Jul 30, 2022
@usualoma usualoma deleted the feat-context-variable-map branch July 30, 2022 20:19
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.

3 participants