Skip to content

Conversation

@yanxue-22
Copy link
Contributor

@yanxue-22 yanxue-22 commented Oct 29, 2025

Ticket: DX-2214

This PR attempts to validate this following fix, which resolved an issue introduced by this Regression Commit.

Original Problem

The system encountered an error when parsing certain Arrow Functions with parameters (e.g., (codec) => t.array(codec)), specifically: Error: Error when parsing AdminUpdateShardKeyApiSpec: Unknown identifier codec.

Cause: The parser attempts to inline the function body for optimization. During this process, it failed because the parameters (like codec) were not recognized in the symbol table, leading to the "Unknown identifier" error.
Previous Behavior: The old code immediately returned the parsing error upon inlining failure, preventing it from checking for custom codec definitions as a necessary fallback.


Why the Fix Works (Utilizing E.isRight Check)

The fix modifies the parser's logic to handle inlining failures gracefully, allowing it to proceed to the custom codec check:

  1. Instead of immediately returning the result of parseFunctionBody(...), the code now stores the result in a variable (bodyResult).
  2. It then uses an E.isRight(bodyResult) check.
    • If successful (right): The successful result is returned.
    • If it fails (left/error): The code does not return immediately. It continues execution, allowing it to reach the subsequent logic that checks for custom codec definitions.

This ensures that even if the inlining optimization fails, the parser still attempts to find a valid definition using custom codecs. Predefined codecs are found in const knownImport = project.resolveKnownImport(imp.from, imp.importedName); inside the codecIdentifier function.


Testing the Fix

The test validates that the type parser correctly falls back to using a pre-defined custom codec for a field utilizing an arrow function with parameters (arrayFromArrayOrSingle(NonEmptyString)), ensuring it resolves to an array type without throwing an Unknown identifier codec error.

@yanxue-22 yanxue-22 changed the title test: add comprehensive tests for arrow function fallback behavior test: add tests for arrow function fallback behavior Oct 29, 2025
@yanxue-22 yanxue-22 force-pushed the DX-2214-Test-Fix-For-Unknown-identifer-codec branch 5 times, most recently from f8824ed to bf9c320 Compare November 4, 2025 00:50
Check if parseFunctionBody() succeeded before returning, allowing parser
to use custom codec definitions when function contains parameter references.
@yanxue-22 yanxue-22 force-pushed the DX-2214-Test-Fix-For-Unknown-identifer-codec branch from bf9c320 to 465569a Compare November 4, 2025 01:07
@yanxue-22 yanxue-22 marked this pull request as ready for review November 4, 2025 01:14
@yanxue-22 yanxue-22 requested a review from a team as a code owner November 4, 2025 01:14
@starfy84 starfy84 requested a review from Copilot November 11, 2025 15:34
Copilot finished reviewing on behalf of starfy84 November 11, 2025 15:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a test to validate the fix for arrow function parameter parsing fallback behavior. The test ensures that when the parser fails to inline arrow function bodies with parameters, it correctly falls back to using predefined custom codec definitions instead of throwing an "Unknown identifier" error.

Key changes:

  • Adds a new test case that validates the parser's fallback mechanism for arrow functions with parameters
  • Test creates a realistic scenario with arrayFromArrayOrSingle(NonEmptyString) to verify the fallback works correctly

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@starfy84 starfy84 left a comment

Choose a reason for hiding this comment

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

@yanxue-22 I think copilot's suggestions broke your PR 😆

The suggestions are correct though. Using required doesn't give TestProject the correct types (it gets typed as any).

The test looks good! But I was wondering if the original fix was correct in the first place 🤔 . Or maybe there's a flaw with the original implementation. The Why the Fix Works section isn't really convincing me too much as to why exactly we need the fallback in the first place. We can go over this during collab!

Comment on lines +39 to +44
const knownImports = {
'.': {
arrayFromArrayOrSingle: (_: any, innerSchema: any) =>
E.right({ type: 'array', items: innerSchema }),
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the type of knownImports doesn't align with the KnownImports type definition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Not entirely confident either now that you point it out - will join collab hours tomorrow!

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