Skip to content

Added extracting of templated params from params #35

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

Merged
merged 2 commits into from
Jul 10, 2023
Merged

Conversation

BEGEMOT9I
Copy link
Contributor

Close #34

Hi! This pull request solves the problem of autoinferring parameters for the params function, but with two edge cases:

  1. If nested templates are used (see example in test file), we can't be sure which of the parameter names will be used. Therefore, for this case, it remains possible to set parameters in the function.
  2. The case of using functions as parameters, for example, the count function, is not processed here. This requires an extension of the return type signature of the TranslationFunction function (even if it will be readable), and will likely require updating the types of all utilities.

package.json Outdated
@@ -15,7 +15,7 @@
],
"scripts": {
"unit": "tsm node_modules/uvu/bin.js . '\\.test\\.(ts|js)$'",
"test": "c8 yarn unit && eslint . && check-dts && size-limit",
"test": "c8 yarn unit && eslint . --report-unused-disable-directives && check-dts && size-limit",
Copy link
Member

Choose a reason for hiding this comment

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

This change should be in different PR

@@ -24,6 +30,12 @@ interface Params {
* @param input Template string.
* @return Transform for translation.
*/
<Input extends string>(input: Input): TranslationFunction<
Copy link
Member

Choose a reason for hiding this comment

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

In Router we were forced to define argument as const to be sure that TS will use "/posts/:id" type instead of just string https://github.com/nanostores/router/blob/main/index.d.ts#L132C30-L132C35

Why we don’t need it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bcs in router types it's needed to narrow types for the other consumers, that will be working with returned type. Here I don't see cases why it might be necessary, but I might be wrong)

Choose a reason for hiding this comment

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

I was passing by and wanted to give a hint that using canonical URLs will be safer in the future. Just press y and the URL will become this: https://github.com/nanostores/router/blob/e927e23d509d1cde4140653071743a3c370de70b/index.d.ts#L132C30-L132C35

? { [key in Param]: string | number } & ExtractTemplateParams<Pre> &
ExtractTemplateParams<Post>
: {}

interface Params {
/**
* Add `{name}` parameters to translation strings.
Copy link
Member

Choose a reason for hiding this comment

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

We will need also to change docs in TypeDoc example and in README.md

@BEGEMOT9I BEGEMOT9I requested a review from ai July 10, 2023 19:36
@BEGEMOT9I
Copy link
Contributor Author

@ai wdyt about adding a case with params and a count function to the demo?

@ai ai merged commit 9300588 into nanostores:main Jul 10, 2023
@ai
Copy link
Member

ai commented Jul 10, 2023

Thanks. Released at 0.11.

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.

Generate types for params() automatically by TS string parsing
3 participants