fix: Expression extension failing with optional chaining #5370
Conversation
n8n-assistant
bot
added
core
valya
force-pushed
the
pay-105-failed-to-extend-syntax-error-on
branch
from
dcb7fd9 to
cd553d4
Compare
| function parseWithEsprimaNext(source: string, options?: any): any { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-argument | ||
| const ast = esprimaParse(source, { | ||
| loc: true, | ||
| locations: true, | ||
| comment: true, | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| range: getOption(options, 'range', false), | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| tolerant: getOption(options, 'tolerant', true), | ||
| tokens: true, | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| jsx: getOption(options, 'jsx', false), | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| sourceType: getOption(options, 'sourceType', 'module'), | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| } as any); | ||
|
|
||
| return ast; | ||
| } |
There was a problem hiding this comment.
Minor details: We could type options as esprima.Config and assert the return types of getOption (instead of ESLint exceptions) and return the result directly without assigning.
|
|
||
| let currentChain = 1; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-argument |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| typeof window !== 'object' ? 'global' : 'window', |
There was a problem hiding this comment.
If we define Window under global the exceptions should not be needed.
| const buildCancelCheckWrapper = (node: ExpressionKind): ExpressionKind => { | ||
| return types.builders.conditionalExpression( | ||
| types.builders.binaryExpression( | ||
| '===', | ||
| cancelMemberExpression, | ||
| types.builders.booleanLiteral(true), | ||
| ), | ||
| undefinedIdentifier, | ||
| node, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Can we define functions like outside of visitChainExpression()? To avoid defining them on every run and to slim down this visitor method.
There was a problem hiding this comment.
They're defined in there to avoid having to pass in a bunch of arguments or create a bunch of expressions multiple times. This is less of an issue with the current implementation and was more of an issue earlier on. I still prefer this way as I think it makes the rest of the code a lot cleaner.
Speaking of defining them on every run. I think we should add a cache (like tmpl does) for expressions because we're performing these transforms every time an extended expression is run. My main concern is the size of the cache, I'm not sure if it'd be a problem on the backend but it might cause issues over long period of time on the frontend. Although we're likely talking in the 0-10MB range over a long session of editing expressions.
| // Just disabling all eslint rules for now. | ||
| // eslint-disable-next-line | ||
| return inputAny[functionName](...args); | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment |
There was a problem hiding this comment.
Maybe we can make the exceptions to @typescript-eslint/no-unsafe-return and @typescript-eslint/no-explicit-any (and any others that happen frequently here) file-level for better readability? Files doing AST manipulation are rare so this should be okay.
There was a problem hiding this comment.
I'm personally not a fan of it just because I like to be reminded when I'm using any but if you think it'd help with readability then I'm OK doing it at a file level.
* wip * fix: working optional chaining polyfill * fix: polyfill optional chaining on extended functions * test: add optional chaining tests
|
Got released with |
No description provided.