This repository has been archived by the owner on May 22, 2024. It is now read-only.
feat: support multiple paths in V2 functions #1586
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱ Benchmark resultsComparing with d98dd20 largeDepsEsbuild: 2.8s⬆️ 7.85% increase vs. d98dd20 Legend
largeDepsNft: 9.3s⬆️ 18.07% increase vs. d98dd20 Legend
largeDepsZisi: 16s⬆️ 7.41% increase vs. d98dd20 Legend
|
lukasholzer
previously approved these changes
Oct 5, 2023
| @@ -262,7 +262,7 @@ describe('V2 API', () => { | |||
| }) | |||
|
|
|||
| test('With an invalid pattern', () => { | |||
| expect.assertions(4) | |||
| expect.assertions(8) | |||
There was a problem hiding this comment.
I've split this into two tests now.
| } | ||
|
|
||
| export const config = { | ||
| path: ["/store/:category/products/:product-id", "/super-awesome-campaign"] |
There was a problem hiding this comment.
can we make the second one a named group as well?
- Can we add a test for what if both routes are the same?
- What if I add falsy values like
nullorundefinedto the array? Do we throw type errors if numbers or whatever is provided? (You have a non nullable function but no test cases for it)
There was a problem hiding this comment.
I've added more tests to capture this.
| const paths = Array.isArray(input) ? input : [input] | ||
| const routes = paths.map((path) => getRoute(path, functionName, methods ?? [])).filter(nonNullable) | ||
|
|
||
| return routes |
There was a problem hiding this comment.
I would do return [...new Set(routes)] to remove duplicates
There was a problem hiding this comment.
We can't do it on routes because it's an array of objects. I've done it on paths.
| } catch { | ||
| throw new FunctionBundlingUserError(`'${path}' is not a valid path according to the URLPattern specification`, { | ||
| functionName, | ||
| runtime: RUNTIME.JAVASCRIPT, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| export const getRoutes = (input: unknown, functionName: string, methods: string[]): Route[] => { |
There was a problem hiding this comment.
NIT: would love a small jsdoc that explains the parameters and the method. So that a tipsy on call engineer at 2am in the morning can understand it as well :D
| return [] | ||
| } | ||
|
|
||
| const getRoute = (path: unknown, functionName: string, methods: string[]): Route | undefined => { |
There was a problem hiding this comment.
NIT: small jsdoc would be nice
|
@lukasholzer I think I addressed all your feedback. I ended up having to make some changes in the AST parsing, because when processing arrays we were only looking for strings, so any other primitive values were ignored (so we couldn't throw a type error if people tried to use them). This has now been addressed. |
lukasholzer
approved these changes
Oct 5, 2023
Skn0tt
pushed a commit
to netlify/build
that referenced
this pull request
May 21, 2024
…-it#1586) * feat: support multiple paths in V2 functions * refactor: remove unnecessary check * refactor: support more primitive types * fix: de-duplicate paths * feat: look for `null`
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Allow an array of strings in the
pathin-source configuration property of V2 functions.The types already allow this, as well as the rest of the infrastructure.