feat: support CommonJS syntax in V2 functions #1585
Conversation
⏱ Benchmark resultsComparing with 1ea5222 largeDepsEsbuild: 2.5s⬇️ 2.05% decrease vs. 1ea5222 Legend
largeDepsNft: 7.6s⬇️ 2.12% decrease vs. 1ea5222 Legend
largeDepsZisi: 15.3s⬆️ 1.03% increase vs. 1ea5222 Legend
|
| schedule?: string | ||
| methods?: string[] | ||
| } | ||
|
|
||
| export interface StaticAnalysisResult extends ISCValues { |
There was a problem hiding this comment.
We were using the term "ISC" to contain properties that are not part of the in-source configuration spec, like invocationMode or runtimeAPIVersion. They were grouped together because we extract both those properties and the ISC properties using the same static analysis step, but functionally they are different things.
So now we have a ISCValues type that holds just the in-source configuration properties, and a StaticAnalysisResult that holds those plus some additional metadata properties we'll extract using the static analysis mechanism.
A big part of this diff is just renaming ISC-related terms to a more generic "static analysis result".
src/utils/cache.ts
Outdated
| // NFT cache, which should not be used in zisi and only supplied to NFT | ||
| // this cache shares the file cache with zisi | ||
| nftCache: NFTCache | ||
|
|
||
| // Used by `get-tsconfig` for caching the retrieval of `tsconfig.json` files |
There was a problem hiding this comment.
NIT: class properties should always be annotated with JSDoc to benefit from this documentation in the IntelliSense when you use it with this.tsConfigCache
| * 4. The module format syntax used in the file: if any `import` or `export` | ||
| * declarations are found, this is ESM; if not, this is CJS | ||
| */ | ||
| export const traverseNodes = (nodes: Statement[], getAllBindings: BindingMethod) => { |
src/runtimes/node/parser/exports.ts
Outdated
| node.type === 'ImportDeclaration' || | ||
| node.type === 'ExportNamedDeclaration' || | ||
| node.type === 'ExportDefaultDeclaration' || | ||
| node.type === 'ExportAllDeclaration' |
There was a problem hiding this comment.
oh, this looks a little bit brittle to me is this the AST from esbuild? – according to the naming I think it is an acorn AST. Are there enums for the values?
But can we define the inputModule format to ESM based on that? Whats about typescript or is this already the transpiled code we are parsing here?
There was a problem hiding this comment.
oh, this looks a little bit brittle to me is this the AST from esbuild? – according to the naming I think it is an acorn AST. Are there enums for the values?
esbuild doesn't expose an AST. This is from Babel, as you can see here:
.Can you elaborate on why you think this is brittle? The nodes are fully typed, so if there's a mismatch in this or in a future version of the AST lexicon, we'll know?
But can we define the inputModule format to ESM based on that? Whats about typescript or is this already the transpiled code we are parsing here?
As I explained in the PR body, we're not trying to infer the module type that we'll emit. We're simply trying to see what kind of syntax was used when authoring the function. If we see an import or an export, we know it's ESM syntax.
For TypeScript functions this won't matter, but we can use this information to detect when people are writing a .js function with ESM without a type: "module". That's the main purpose of surfacing this information.
What parts of the codebase do you feel are lacking documentation? |
|
After discussing with @lukasholzer, we're not sure this is still needed in its current form. I still think some of the cleaning up we're doing here would be great to land, so I'm moving to draft until I have time to pick this up again. |
eduardoboucas
changed the title
tsConfig property to listFunctions output
| * Loads a file at a given path, parses it into an AST, and returns a series of | ||
| * data points, such as in-source configuration properties and other metadata. | ||
| */ | ||
| export const parseFile = async ( |
There was a problem hiding this comment.
This was renamed from findISCDeclarationsInPath. No functionality has changed.
| @@ -101,27 +112,28 @@ export const findISCDeclarations = ( | |||
| let scheduleFound = false | |||
|
|
|||
| const getAllBindings = createBindingsMethod(ast.body) | |||
| const { configExport, defaultExport, handlerExports } = getExports(ast.body, getAllBindings) | |||
| const isV2API = handlerExports.length === 0 && defaultExport !== undefined | |||
| const { configExport, handlerExports, hasDefaultExport, inputModuleFormat } = traverseNodes(ast.body, getAllBindings) | |||
There was a problem hiding this comment.
We were not using the contents of defaultExport for anything, just checking whether it exists or not. So I've changed it to a boolean that is now true if ESM or CJS default exports are found.
| } | ||
|
|
||
| // Finds the main handler export in a CJS AST. | ||
| const getMainExportFromCJS = (node: Statement) => { | ||
| const getCJSExports = (node: Statement, name: string) => { |
There was a problem hiding this comment.
This is now generic so it can look for exports with different names.
| * with a variable declaration (`export const foo = "bar"`), but also resolve | ||
| * bindings and find things like `const baz = "1"; export { baz as foo }`. | ||
| */ | ||
| const getNamedESMExport = (node: Statement, name: string, getAllBindings: BindingMethod) => { |
There was a problem hiding this comment.
Renamed from getMainExportFromESM. This is now generic so it can look for exports with different names.
| * - `export { handler }`, or | ||
| * - `export { x as "handler" }` | ||
| */ | ||
| const isNamedExport = (node: ExportNamedDeclaration['specifiers'][number], name: string): node is ExportSpecifier => { |
There was a problem hiding this comment.
Renamed from isHandlerExport. This is now generic so it can look for exports with different names.
| name: string, | ||
| getAllBindings: BindingMethod, | ||
| ) => { | ||
| const specifier = specifiers.find((node) => isNamedExport(node, name)) |
There was a problem hiding this comment.
here you get only the named exports what about doing an object literal export over a default?
const handler = () => {}
export default {
handler
}There was a problem hiding this comment.
same would apply to the cjs where it's:
const handler = () => {}
exports.default {
handler
}
…p-it#1585) * feat: add `inputModuleFormat` property * feat: add `tsConfig` to `listFunctions` output * chore: update tests * chore: update tests * chore: update snapshots * chore: update snapshot * refactor: rename methods * feat: return `jsModuleFormat` property * feat: add bundled paths to `inputs` * feat: force bundler to NFT in V2 functions * chore: fix test * refactor: remove `tsconfig` property * refactor: remove traces of `tsconfig` * feat: support CJS in V2 functions * refactor: tidy up
Summary
Adds support for CommonJS syntax when authoring V2 functions, including support for the
configobject in CJS.It also adds a new
inputModuleFormatproperty that containscjsoresmbased on the syntax used to author the function (which may differ from the module format that is actually generated, represented byoutputModuleFormatfrom #1589).Most of the PR is actually a cleanup around ISC and the static analysis code. I've added some inline comments that will hopefully make it easier to review, but feel free to ask anything that isn't clear.