Skip to content

Conversation

eduardoboucas
Copy link
Member

@eduardoboucas eduardoboucas commented Jul 4, 2025

Summary

Follow-up to #6487, exposing the list of generated functions to consumers of @netlify/build via the default export and the startDev method.

This will be used by the CLI to learn about any functions that were generated and that need to be added to the list of function paths.

I ended up changing the internals a little bit such that we keep passing returnValues around internally, but what we expose externally is a generatedFunctions object. The generation of this object has been moved to a separate file, which is now reused by the functions core step and the logic that prints functions. This is for unification and simplicity purposes, no functional changes are expected.

A picture of a cute animal (not mandatory, but encouraged)

🦅

Copy link
Contributor

github-actions bot commented Jul 4, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

Copy link
Contributor

github-actions bot commented Jul 4, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@eduardoboucas eduardoboucas marked this pull request as ready for review July 4, 2025 16:14
@eduardoboucas eduardoboucas requested a review from a team as a code owner July 4, 2025 16:14
serhalp
serhalp previously approved these changes Jul 4, 2025
Copy link
Member

@serhalp serhalp left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +256 to +260
const result: Record<string, GeneratedFunction[]> = {}

for (const func of generatedFunctions) {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
result[func.generator.name] = result[func.generator.name] || []
Copy link
Member

@serhalp serhalp Jul 4, 2025

Choose a reason for hiding this comment

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

FWIW this is because given f: Record<string, Foo> with noUncheckedIndexAccess disabled, f[string] results in the type Foo, not Foo | undefined. ESLint is being pedantically correct here (arguably).

You can avoid the suppression this way:

Suggested change
const result: Record<string, GeneratedFunction[]> = {}
for (const func of generatedFunctions) {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
result[func.generator.name] = result[func.generator.name] || []
const result: Record<string, GeneratedFunction[] | undefined> = {}
for (const func of generatedFunctions) {
result[func.generator.name] = result[func.generator.name] || []

Copy link
Member Author

Choose a reason for hiding this comment

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

But that's not the type I want to return. I don't want consumers of this method to think that some keys will have the value undefined, because that shouldn't be the case.

Copy link
Member

Choose a reason for hiding this comment

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

It's problematic either way. It's just a limitation of TypeScript's type system without noUncheckedIndexAccess:

  • With your type, a user dereferencing result['kjfandsakj'] will get GeneratedFunction[].
  • With my type, a user iterating over result's own keys and dereferencing result[key] will get GeneratedFunction[] | undefined.

Both cases are not what we intend to express 😞. The latter is safer, but way more annoying.

https://claude.ai/share/a8fbb3cb-bd72-4761-a2e6-1d55e876889c

The only good solution is enabling noUncheckedIndexAccess.

(To be clear, this is absolutely nonblocking tangential rambling—please proceed with merging!)

Copy link
Contributor

github-actions bot commented Jul 7, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@eduardoboucas eduardoboucas requested a review from serhalp July 7, 2025 12:40
@eduardoboucas eduardoboucas enabled auto-merge (squash) July 7, 2025 13:51
Copy link
Contributor

github-actions bot commented Jul 7, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@eduardoboucas eduardoboucas merged commit 536630e into main Jul 7, 2025
33 checks passed
@eduardoboucas eduardoboucas deleted the feat/expose-generated-functions branch July 7, 2025 14:06
ndhoule pushed a commit to netlify/cli that referenced this pull request Jul 28, 2025
#### Summary

Loads functions generated by the new `functions.generate` util added in netlify/build#6487 and netlify/build#6525.

It depends on netlify/build#6539, so tests will fail until that lands.
This was referenced Sep 2, 2025
This was referenced Sep 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants