fix: handle multiple snippet parameters with one or more being optional #10833
Conversation
🦋 Changeset detectedLatest commit: f97dbca The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // If this is a type annotation for a function parameter, Acorn-TS will treat subsequent | ||
| // parameters as part of a sequence expression instead, and will then error on optional | ||
| // parameters (`?:`). Therefore replace that sequence with something that will not error. | ||
| parser.template.slice(parser.index).replace(/\?\s*:/g, ':'); |
There was a problem hiding this comment.
I think this should be safe, this combination is only relevant syntax-wise for optional parameters, there's nothing that should get invalid anywhere else with regards Acorn-TS parsing this - or am I overlooking something?
There was a problem hiding this comment.
While it's pretty odd to break things up in the following way, it doesn't support comments:
const f = (n? // foo
: number): number => n ?? 0 + 1
const g = (n? /* foo */ : number): number => n ?? 0 + 1Those are both valid in normal TypeScript, but broken with this solution.
It also still supports invalid parameter orderings, such as the first being optional and the second being required. Not sure if that's a separate issue or one that this aims to fix (if it even is an issue - it might just be the repl not caring).
There was a problem hiding this comment.
Looking into both the points you made:
- Supporting comments here would be rare as this is within the template itself – where no one adds comments.
- The invalid ordering of params isn't such an issue here I don't think. This is something for language tools maybe rather than the parser.
| // If this is a type annotation for a function parameter, Acorn-TS will treat subsequent | ||
| // parameters as part of a sequence expression instead, and will then error on optional | ||
| // parameters (`?:`). Therefore replace that sequence with something that will not error. | ||
| parser.template.slice(parser.index).replace(/\?\s*:/g, ':'); |
There was a problem hiding this comment.
Looking into both the points you made:
- Supporting comments here would be rare as this is within the template itself – where no one adds comments.
- The invalid ordering of params isn't such an issue here I don't think. This is something for language tools maybe rather than the parser.
fixes #10825
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.Tests and linting
pnpm testand lint the project withpnpm lint