feat: detect V2 API using static analysis #1429
Conversation
⏱ Benchmark resultsComparing with 569ebf4 largeDepsEsbuild: 2.2s⬆️ 5.71% increase vs. 569ebf4 Legend
largeDepsNft: 7.7s⬆️ 3.85% increase vs. 569ebf4 Legend
largeDepsZisi: 14.9s⬆️ 2.01% increase vs. 569ebf4 Legend
|
| const iscExports = mainExports | ||
| const { defaultExport, handlerExports } = getExports(ast.body, getAllBindings) | ||
|
|
||
| if (featureFlags.zisi_functions_api_v2 && handlerExports.length === 0 && defaultExport !== undefined) { |
There was a problem hiding this comment.
Now the feature flag is used to control whether the detection mechanism kicks in. We should be able to roll it out safely to everyone and remove the flag entirely as a quick follow-up.
| @@ -76,6 +76,7 @@ export const parseExpression = ({ | |||
| resolveDir: string | |||
| }) => { | |||
| const { program } = parse(rawExpression, { | |||
| plugins: ['typescript'], | |||
There was a problem hiding this comment.
Because we're now parsing the source before bundling/transpiling.
There was a problem hiding this comment.
parseExpression is only used in esbuild when parsing non-static imports. Do we need the ts plugin there? At least it wasn't necessary up until now.
| @@ -67,6 +59,17 @@ const zipFunction: ZipFunction = async function ({ | |||
| return { config, path: destPath } | |||
| } | |||
|
|
|||
| const inSourceConfig = await findISCDeclarationsInPath(mainFile, name, featureFlags) | |||
There was a problem hiding this comment.
I'd like to call out this change, since we're now parsing ISC before bundling. This means we'll start parsing ESM, CJS and TS rather than the bundled/transpiled CJS. I don't anticipate any problems with this, but it's not something I can easily wrap with a feature flag, so something to keep an eye on.
There was a problem hiding this comment.
We need to update the babel-parser in the future though, as we otherwise do not support the latest ES standard. It is currently locked at an old version because of previous performance problems. Last time I checked beginning of this year or end of last, the performance problems were still there I think.
But I think the problems are because of the zisi bundler and not necessarily the isc parsing.
| // 2. Default function export | ||
| // 3. Named `config` object export | ||
| export const getExports = (nodes: Statement[], getAllBindings: BindingMethod) => { | ||
| const handlerExports: ISCExport[] = [] |
There was a problem hiding this comment.
just out of curiosity - could you give an example for a file that has multiple exports named handler? I thought that's impossible, but maybe i'm wrong. Or is this an array to make the code for CJS/ESM easier?
There was a problem hiding this comment.
exports.handler = () => {}
module.exports.handler = () => {}Or even one of these variants repeated multiple times. It's true that only one will actually be evaluated, but at this stage we're just processing the AST so multiple declarations will lead to multiple nodes.
There was a problem hiding this comment.
Makes sense, so this can only happen for CJS code. I'm wondering if it makes sense to throw / print a warning if this occurs - it'd make our code a tad easier + help catch user bugs. But maybe it'd break weird bundles that some frameworks emit 🤔
| @@ -55,6 +55,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@babel/parser": "7.16.8", | |||
| "@babel/plugin-syntax-typescript": "^7.21.4", | |||
There was a problem hiding this comment.
There is no need to install this. This plugin is for babel itself, but we are only using the parser, which has no concept of plugin packages. For the parser plugins are simply strings that enable features.
| @@ -67,6 +59,17 @@ const zipFunction: ZipFunction = async function ({ | |||
| return { config, path: destPath } | |||
| } | |||
|
|
|||
| const inSourceConfig = await findISCDeclarationsInPath(mainFile, name, featureFlags) | |||
There was a problem hiding this comment.
We need to update the babel-parser in the future though, as we otherwise do not support the latest ES standard. It is currently locked at an old version because of previous performance problems. Last time I checked beginning of this year or end of last, the performance problems were still there I think.
But I think the problems are because of the zisi bundler and not necessarily the isc parsing.
| return parseObject(node.declaration.declarations[0].init) | ||
| } | ||
|
|
||
| return {} |
There was a problem hiding this comment.
should we return null or undefined here? Then the check above could be easier
-if (Object.keys(config).length !== 0) {
+if (config) {| // - number | ||
| // - object | ||
| // - string | ||
| const parseObject = (node: ObjectExpression) => |
There was a problem hiding this comment.
I guess we could at some point add support for template literals, but that is not high prio.
| @@ -76,6 +76,7 @@ export const parseExpression = ({ | |||
| resolveDir: string | |||
| }) => { | |||
| const { program } = parse(rawExpression, { | |||
| plugins: ['typescript'], | |||
There was a problem hiding this comment.
parseExpression is only used in esbuild when parsing non-static imports. Do we need the ts plugin there? At least it wasn't necessary up until now.
…#1429) * feat: detect V2 API using static analysis * chore: add test case * feat: propagate runtime API version * feat: support `config` object * refactor: simplify code * chore: improve comments * refactor: safely parse source * refactor: rename property * chore: update lock file * chore: update tests * chore: update tests * chore: re-enable V2 tests * refactor: bail when default export is found
Summary
Part of https://github.com/netlify/pod-compute/issues/476.