-
Notifications
You must be signed in to change notification settings - Fork 13
feat: remove logic in compile.ts to flatten references to expressions #376
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
Conversation
✅ Deploy Preview for effortless-malabi-1c3e77 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| if (node === undefined) { | ||
| if (defaultValue === undefined) { | ||
| return []; | ||
| } else { | ||
| return resolve(defaultValue, undefined); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong? Default value can only be determined at runtime.
const { a = func } = b ? { a: func2 } : { a: undefined }
a is func if b is falsey (runtime) or func2 is undefined (maybe runtime)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We care calling resolve on the defaultValue which can return 0, 1 or many possible values. In your case, we would have logic for the ConditionExpr that returns [{a: func2}, {a: undefined}] which then distributes to [func2, undefined] which then unions with a = func to create [func, func2, undefined] as the set of all possible values that a can be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's expand on this in #310 ?
src/integration.ts
Outdated
| return resolve(node.parent, node.initializer).flatMap((pattern) => { | ||
| if (isIdentifier(node.name)) { | ||
| return [pattern[node.name.name]]; | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is pattern here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad name - it's one of the possible values that node.initializer can be
| * for (const a of tables) { | ||
| * const b = a; | ||
| * // ^ [table1, table2] | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this implemented? I don't see any code that could return an array of more than 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, no. I implemented only the cases I needed for the tests to pass - incremental progress. I added the comments as I was thinking it through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to update the interpreter logic to support this case so it didn't matter.
Partial #374