Skip to content
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

(aws-lambda-nodejs): esbuild does not support emitting typescript decorator metadata #13767

Closed
whimzyLive opened this issue Mar 24, 2021 · 19 comments · Fixed by #16543
Closed
Labels
@aws-cdk/aws-lambda-nodejs effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@whimzyLive
Copy link
Contributor

Somewhere in the recent version of 'aws-lambda-nodejs' module, the default bundler for aws-lambda-nodejs was changed from 'parcel' to 'esbuild'. Bundling performance has improved greatly but all my handlers are broken now (all the ones that were relying on ts metadata reflaction).

This has to do with how esbuild transpiles typescript code, as stated here by the author of esbuild, emitDecoratorMetadata is not and will not be supported by esbuild, but this would break all other libraries that rely on the availability of run time metadata.
like: TypeORM, TypeDORM, Nestjs.

I think aws-lambda-nodejs should auto handle pre-transpiling files with decorators using tsc and later continue with esbuild. (Similar suggested by the author of esbuild too.)

Reproduction Steps

Trying to bundle a simple nodejs project using Reflect metadata api should demonstrate the problem.

What did you expect to happen?

when bundling code with aws-lambda-nodejs decorator metadata should be reflected properly.

What actually happened?

Decorator metadata is not reflected in the fine js bundle.

If this is something aws-lambda-nodejs should handle, I can do a PR.

Environment

  • **CDK CLI Version :1.91
  • **Framework Version:1.91
  • **Node.js Version:14.15
  • **OS :linux
  • **Language (Version):TypeScript (3.9.9)

This is 🐛 Bug Report

@whimzyLive whimzyLive added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 24, 2021
@jogold
Copy link
Contributor

jogold commented Mar 24, 2021

While it's not really a solution to your problem, one way to get around it is to use a command hook to run a first "transpilation" before bundling.

@whimzyLive
Copy link
Contributor Author

I thought of doing that, but that would then defeat the purpose of using esbuild altogether (build performance), but that's an option.

@whimzyLive
Copy link
Contributor Author

whimzyLive commented Mar 29, 2021

@jogold have done a quick patch for this ATM, but would be good to include that in the official nodejs func construct, as this will be something everyone will need, at least those relying on any of the libs that use ts reflection API.
what are your thoughts?

here is the quick patch that I did.

diff --git a/node_modules/@aws-cdk/aws-lambda-nodejs/lib/build.js b/node_modules/@aws-cdk/aws-lambda-nodejs/lib/build.js
new file mode 100644
index 0000000..14a7e0e
--- /dev/null
+++ b/node_modules/@aws-cdk/aws-lambda-nodejs/lib/build.js
@@ -0,0 +1 @@
+const esbuild = require('esbuild')
\ No newline at end of file
diff --git a/node_modules/@aws-cdk/aws-lambda-nodejs/lib/bundling.js b/node_modules/@aws-cdk/aws-lambda-nodejs/lib/bundling.js
index 82d529b..96ee454 100644
--- a/node_modules/@aws-cdk/aws-lambda-nodejs/lib/bundling.js
+++ b/node_modules/@aws-cdk/aws-lambda-nodejs/lib/bundling.js
@@ -34,7 +34,43 @@ class Bundling {
                     ESBUILD_VERSION: (_h = props.esbuildVersion) !== null && _h !== void 0 ? _h : ESBUILD_VERSION,
                 },
             }) : cdk.BundlingDockerImage.fromRegistry('dummy'); // Do not build if we don't need to
-        const bundlingCommand = this.createBundlingCommand(cdk.AssetStaging.BUNDLING_INPUT_DIR, cdk.AssetStaging.BUNDLING_OUTPUT_DIR);
+        
+
+        const osPlatform = os.platform();
+        const npx = osPlatform === 'win32' ? 'npx.cmd' : 'npx';
+        let transpileCommand = ''
+        if(this.isTypescriptEntry(this.props.entry)) {
+            // if typescript entry was found, check if the emitDecoratorMetadata was set to true;            
+            const tsConfigPath = this.props.tsconfig ? 
+                path.resolve(this.props.tsconfig) : 
+                path.resolve(util_1.findUp('tsconfig.json', path.dirname(this.props.entry)));
+    
+            if(tsConfigPath) {
+                const tsConfig = require(tsConfigPath);                
+                // when asked to emit decorator metadata, compile using tsc
+                if(tsConfig && tsConfig.compilerOptions && tsConfig.compilerOptions.emitDecoratorMetadata) {
+
+                    // add tsc based compilation
+                    transpileCommand = [
+                        npx, 
+                        'tsc'
+                    ].join(' ');
+
+                    if(tsConfig.compilerOptions.outDir) {
+                        this.relativeEntryPath = `${tsConfig.compilerOptions.outDir}/${this.relativeEntryPath}`
+                    }
+                    this.relativeEntryPath = this.relativeEntryPath
+                        .replace(/\.(tsx)$/, '.jsx')
+                        .replace(/\.(ts)$/, '.js');
+                      
+                    console.log(`Relative entry path resolved to "${this.relativeEntryPath}"`)
+                }
+    
+            }
+
+        }
+
+        const bundlingCommand = this.createBundlingCommand(cdk.AssetStaging.BUNDLING_INPUT_DIR, cdk.AssetStaging.BUNDLING_OUTPUT_DIR, osPlatform, transpileCommand);
         this.command = ['bash', '-c', bundlingCommand];
         this.environment = props.environment;
         // Bundling sets the working directory to cdk.AssetStaging.BUNDLING_INPUT_DIR
@@ -42,8 +78,7 @@ class Bundling {
         this.workingDirectory = '/';
         // Local bundling
         if (!props.forceDockerBundling) { // only if Docker is not forced
-            const osPlatform = os.platform();
-            const createLocalCommand = (outputDir) => this.createBundlingCommand(projectRoot, outputDir, osPlatform);
+            const createLocalCommand = (outputDir) => this.createBundlingCommand(projectRoot, outputDir, osPlatform, transpileCommand);
             this.local = {
                 tryBundle(outputDir) {
                     var _a;
@@ -82,10 +117,20 @@ class Bundling {
     static clearRunsLocallyCache() {
         this.runsLocally = undefined;
     }
-    createBundlingCommand(inputDir, outputDir, osPlatform = 'linux') {
+
+    isTypescriptEntry(entryFile) {
+        if(/\.(tsx?)$/.test(entryFile)) {
+            return true;
+        }    
+        return false;
+    }
+    
+
+    createBundlingCommand(inputDir, outputDir, osPlatform = 'linux', transpileCommand = '') {
         var _a, _b, _c, _d, _e, _f, _g, _h, _j;
         const pathJoin = osPathJoin(osPlatform);
         const npx = osPlatform === 'win32' ? 'npx.cmd' : 'npx';
+
         const loaders = Object.entries((_a = this.props.loader) !== null && _a !== void 0 ? _a : {});
         const defines = Object.entries((_b = this.props.define) !== null && _b !== void 0 ? _b : {});
         const esbuildCommand = [
@@ -132,6 +177,7 @@ class Bundling {
             ]);
         }
         return chain([
+            transpileCommand,
             ...(_e = (_d = this.props.commandHooks) === null || _d === void 0 ? void 0 : _d.beforeBundling(inputDir, outputDir)) !== null && _e !== void 0 ? _e : [],
             esbuildCommand,
             ...(_g = (this.props.nodeModules && ((_f = this.props.commandHooks) === null || _f === void 0 ? void 0 : _f.beforeInstall(inputDir, outputDir)))) !== null && _g !== void 0 ? _g : [],
diff --git a/node_modules/@aws-cdk/aws-lambda-nodejs/lib/function.js b/node_modules/@aws-cdk/aws-lambda-nodejs/lib/function.js
index e82af8c..cc5bcd0 100644
--- a/node_modules/@aws-cdk/aws-lambda-nodejs/lib/function.js
+++ b/node_modules/@aws-cdk/aws-lambda-nodejs/lib/function.js
@@ -41,7 +41,9 @@ class NodejsFunction extends lambda.Function {
             depsLockFilePath = lockFile;
         }
         // Entry and defaults
-        const entry = path.resolve(findEntry(id, props.entry));
+        let entry = path.resolve(findEntry(id, props.entry));
+        
+
         const handler = (_c = props.handler) !== null && _c !== void 0 ? _c : 'handler';
         const defaultRunTime = util_1.nodeMajorVersion() >= 12
             ? lambda.Runtime.NODEJS_12_X
@@ -95,6 +97,7 @@ function findEntry(id, entry) {
     }
     throw new Error('Cannot find entry file.');
 }
+
 /**
  * Finds the name of the file where the `NodejsFunction` is defined
  */

@eladb
Copy link
Contributor

eladb commented Apr 13, 2021

@whimzyLive, if I understand correctly, the feature request here is to add a feature to lambda-nodejs which uses tsc for transpiling .ts to .js before bundling?

@eladb eladb added feature-request A feature should be added or improved. p1 effort/medium Medium work item – several days of effort and removed bug This issue is a bug. labels Apr 13, 2021
@eladb eladb removed their assignment Apr 13, 2021
@whimzyLive
Copy link
Contributor Author

@eladb Yes, that is correct. I will do a PR soon addressing this issue.

@TomiTakussaari
Copy link

TomiTakussaari commented Apr 26, 2021

I ended up solving this issue (for my usecase) by allowing usage of esbuild plugins.

It works by having separate script (esbuild.js) for running esbuild, and CDK executing that script instead of executing esbuild cli. Esbuild configuration is passed from CDK to script as a (base64-encoded JSON) CLI argument.

This means that all esbuild configuration options work, which means that I can use esbuild plugin that provides emitDecoratorMetadata support.

I don't know if this is something that CDK could also do, in eg. Docker build., but it seems to work in my usecase, which is using local esbuild. This also allows us to reuse esbuild configuration when building lambdas for eg. local testing with SAM local api.

Relevant parts in quick and dirty way:

// bundling.ts
public createBundlingCommand(
    inputDir: string,
    outputDir: string,
    osPlatform: NodeJS.Platform = "linux"
  ): string {
    const pathJoin = osPathJoin(osPlatform);
    const esbuildScript = pathJoin(__dirname, "./esbuild.js");

    const esBuildConfig: Record<string, any> = {
      entryPoints: [pathJoin(inputDir, this.relativeEntryPath)],
      loader: this.props.loader ?? {},
      define: this.props.define ?? {},
      plugins: this.props.plugins,
      platform: "node",
      bundle: true,
      target: this.props.target,
      outfile: pathJoin(outputDir, "index.js"),
      minify: this.props.minify,
      external: this.externals,
      logLevel: this.props.logLevel,
      sourceMap: this.props.sourceMap,
      keepNames: this.props.keepNames,
      tsconfig: this.relativeTsconfigPath
        ? pathJoin(inputDir, this.relativeTsconfigPath)
        : undefined,
      banner: this.props.banner,
      footer: this.props.footer,
    };
    const configBuffer = Buffer.from(JSON.stringify(esBuildConfig));
    const esbuildCommand: string = [
      "node",
      esbuildScript,
      "--config",
      configBuffer.toString("base64"), // probably could pass JSON string also, but this felt safer.
    ].join(" ");
    ...
}
// esbuild.js
const esbuild = require("esbuild");

const args = process.argv.slice(2);
if (args[0] !== "--config" || !args[1]) {
    throw new Error("--config parameter is required");
}
const configValue = Buffer.from(args[1], "base64");
const config = JSON.parse(configValue.toString("utf8"));
function mapPlugins(plugins) {
    return plugins.map(({ packageName, config, functionName }) => {
       const plugin = require(packageName);
       const func = plugin[functionName]
       return func(config || {});
    });
}
config.plugins = config.plugins ? mapPlugins(config.plugins) : [];
esbuild.build(config).then(() => {
    console.info("Esbuild successful");
}).catch((error) => {
    console.error("Esbuild failed", error);
    process.exit(1);
})

@bpred754
Copy link

@whimzyLive Have you been able to create a PR for this?

@whimzyLive
Copy link
Contributor Author

whimzyLive commented May 4, 2021

@bpred754 not yet, totally forgot about this one, thanks for the reminder. will try to get it in by the end of this week, until then feel free to use the path file.

@deldrid1
Copy link

+1 for @whimzyLive submitting a PR :)

@ryparker ryparker removed the needs-triage This issue or PR still needs to be triaged. label Jun 1, 2021
@deldrid1
Copy link

deldrid1 commented Jul 9, 2021

Just checking in - I would LOVE to drop webpack in my project and move to this, but I'm an esbuild (and CDK) noob... Any chance someone could pick this up or work with me to help get a PR going?

@whimzyLive
Copy link
Contributor Author

whimzyLive commented Jul 12, 2021

@deldrid1 Sorry, wasn't able to get this fix in, but if you want to pick it up, basically everything that needs to get fixed is in the patch that I had pasted above except one change where you will have to take into consideration that a project might be using multiple extended tsconfig.json, in that case, you would want to start by the very first tsconfig.json in the tree and continue merging all configs until you are at the base config.

Otherwise, I will get it done at some point this week. 🤞

@hassanazharkhan
Copy link
Contributor

@whimzyLive any update over this ?

@mergify mergify bot closed this as completed in #16543 Oct 26, 2021
mergify bot pushed a commit that referenced this issue Oct 26, 2021
closes [#13767](#13767)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@modul
Copy link

modul commented Dec 8, 2021

@hassanazharkhan I was waiting on your PR to be merged. I'm trying to use the new preCompilation flag but it doesn't look like tsc is being run. The resulting lambda code is missing metadata.

Am I missing something here?

@hassanazharkhan
Copy link
Contributor

hassanazharkhan commented Dec 8, 2021

@modul You are not missing anything,there seems to be bug with preCompiled flag if you try to bundle using docker it will probably work but not sure however I will try to PR the fix ASAP!

@modul
Copy link

modul commented Dec 13, 2021

@hassanazharkhan sorry to bother you, but do you have any updates on this? Should I open another issue for this to keep better track maybe?

@hassanazharkhan
Copy link
Contributor

@modul Yeah would be nice to have another issue with this bug.

if you are blocked by this, I'd suggest adding extra step in your build process to run tsc and reference the transpiled .js in your nodejs-function

@dobrynin
Copy link

Also looking for this.

@tgjorgoski
Copy link

Hi @hassanazharkhan and others, I'm trying to use the workaround, but not sure what I'm doing wrong.
The situation is this: I have handler.ts file for the lambda, and it references ../dao/Customer.ts which has typeorm annotations.
I transpile both files using tsc, and I see transpiled js files handler.js and Customer.js. When I open Customer.js I see the decorator metadata being present.
I then point the nodejsfunction to the handler.js file. However still when I see the resulting bundle the decorator metadata is not being present in it.
Any other ideas?

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
)

closes [aws#13767](aws#13767)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda-nodejs effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet