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

Add array search params #585

Merged
merged 10 commits into from
Dec 31, 2023
Merged

Add array search params #585

merged 10 commits into from
Dec 31, 2023

Conversation

fehnomenal
Copy link
Contributor

@fehnomenal fehnomenal commented Dec 30, 2023

This is an alternative to #583, where the user can decide whether the array elements should be joined or each have their own occurrence in the search params.

This adds a new option to the explicit search param definition: joinArray. It controls whether .join(',') gets appended in the generated call of the appendSp function.
Otherwise the array gets passed as it is and is turned into many parameters.

fix #580


Side-note: Have you considered not exporting the appendSp string but the function? It would keep the diff smaller if ROUTES.ts would import it from the lib 😅

Copy link
Owner

@jycouet jycouet left a comment

Choose a reason for hiding this comment

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

I kinda like the fact that there is 0 import from the lib 🤣
Yes, diffs are big, like this you take a lot of contribution 😉

packages/vite-plugin-kit-routes/src/lib/plugin.ts Outdated Show resolved Hide resolved
@fehnomenal
Copy link
Contributor Author

Phew, this was harder than expected as we do not know if we have an array during the code generation step. So the "logic" to join has to run always. But that does not work correctly if the value is undefined (String(undefined) => 'undefined').

Let me know what you think!

Copy link
Owner

@jycouet jycouet left a comment

Choose a reason for hiding this comment

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

Great job 💪
Few questions and we are good I think ✅

packages/vite-plugin-kit-routes/src/lib/plugin.ts Outdated Show resolved Hide resolved
packages/vite-plugin-kit-routes/src/lib/format.ts Outdated Show resolved Hide resolved
Copy link
Owner

@jycouet jycouet left a comment

Choose a reason for hiding this comment

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

It needs a changeset and we are good.
Thx for your work.

Let me know if you want to finish it, if not, I can take it from here.

Comment on lines 64 to 65
limit: StringOrUndefined(params.limit),
demo: StringOrUndefined(params.demo),
Copy link
Owner

Choose a reason for hiding this comment

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

It should be

StringOrUndefined(params?.limit),

no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

params is definitively set in line 60. I struggled so long to get the ?. operator to bet used in the correct places 😅

packages/vite-plugin-kit-routes/src/lib/plugin.ts Outdated Show resolved Hide resolved
@fehnomenal
Copy link
Contributor Author

I used minor for the changeset. Is this correct in pre mode?

@jycouet jycouet merged commit b9fc188 into jycouet:main Dec 31, 2023
4 checks passed
@jycouet
Copy link
Owner

jycouet commented Dec 31, 2023

@all-contributors please add @fehnomenal for code

Thx a lot

Copy link
Contributor

@jycouet

I've put up a pull request to add @fehnomenal! 🎉

@fehnomenal fehnomenal deleted the feat/array-params branch December 31, 2023 11:58
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.

[kit-routes] Support array search params
2 participants