powertools-lambda-typescript icon indicating copy to clipboard operation
powertools-lambda-typescript copied to clipboard

Bug (Tracer): unable to build with `tsc` when not using `@middy/core`

Open rreddypally opened this issue 2 years ago • 10 comments

Bug description

EDIT: see this comment for updated info after triage.


The import statement in Tracer.ts for TracerInterface is importing all the modules from the root. https://github.com/awslabs/aws-lambda-powertools-typescript/blob/main/packages/tracer/src/Tracer.ts#L3.

I am trying to use tracer module without the need to install middy.

Expected Behavior

Should be able to use Tracer without the need to import middy when its not used.

Current Behavior

This behavior is forcing to import middy middleware

Possible Solution

import { TracerInterface } from './TracerInterface';

Steps to Reproduce

  1. Install tracer npm module npm i @aws-lambda-powertools/tracer
  2. Import tracer import { Tracer } from "@aws-lambda-powertools/tracer/lib/Tracer";
  3. Add tracer annotation and build fails with error TS2307: Cannot find module '@middy/core' or its corresponding type declarations.

Environment

  • Powertools version used: 1.1.1
  • Packaging format (Layers, npm): npm
  • AWS Lambda function runtime: Node 16

rreddypally avatar Aug 22 '22 08:08 rreddypally

Hi @rreddypally thank you for opening this issue.

I have tried reproducing the issue you describe but unfortunately I am not able to.

In the docs we recommend to import the Tracer directly (i.e. import { Tracer } from '@aws-lambda-powertools/tracer';), however even if I import it like you suggest I am still able to bundle the function, run it successfully on AWS Lambda, and see the annotated segment in the traces.

I have made a screen record in which I create a new project from zero and I set up the latest version of Tracer in a function, then bundle it, and then deploy & run it. As mentioned, I have tested both import statements (the recommended one, and the scoped one), and both build & run successfully.

As you'll see in the video, I never install Middy and I only install esbuild to bundle, and @types/node for types (probably redundant anyway).

https://user-images.githubusercontent.com/7353869/185918256-4d69291f-d083-45e4-a1c8-129e7de1fd13.mp4

Additionally, as a side note, you'll see that the file containing the LambdaInterface never imports Middy, and the same applies to the types file which is the only other imported file. The only Middy-related import is in the middleware file, which should be imported only if you import the captureLambdaHandler middleware, which requires Middy.

Please let me know if this answers your question.

dreamorosi avatar Aug 22 '22 12:08 dreamorosi

EDIT from maintainer: removed zip archive

Hi @dreamorosi, thanks for taking time and record the extensive demo.

I have attached a typescript demo which fails to compile without middy.

Steps:

  1. Unpack the demo
  2. Run npm run build which uses tsc to compile
  3. Get this error. "node_modules/@aws-lambda-powertools/tracer/lib/middleware/middy.d.ts:1:24 - error TS2307: Cannot find module '@middy/core' or its corresponding type declarations."

I think esbuild would have the smarts to eliminate the unused modules while tree shaking which is the reason why your demo works.

Hope this helps.

rreddypally avatar Aug 25 '22 00:08 rreddypally

Unfortunately I'm not allowed to download archives from public websites on my machine.

Could you please provide a sample repo here on GitHub with its content so I can take a look? Thanks for your understanding

dreamorosi avatar Aug 25 '22 05:08 dreamorosi

No problem at all.

Here is the repo: https://github.com/rreddypally/tracer-demo

Let me know if you have trouble accessing it.

rreddypally avatar Aug 25 '22 08:08 rreddypally

Thank you for providing the repo.

I can confirm that I am able to reproduce the issue you describe, which happens only when bundling with tsc instead of esbuild.

I was also able to confirm that the issue affects the Logger & Metrics utilities as well.

We're going to need a bit more time to investigate this though, the fix that you suggested in the PR #1069 does not seem to fix the issue for me, at least when using the standard import { Tracer } from "@aws-lambda-powertools/tracer" import (it does fix it when importing @aws-lambda-powertools/tracer/lib/Tracer but I'd like to see what are our options.

I'm going to rename the issue, after that would you be so kind to please open two more issues one for Metrics & another for Logger with the same title? Would really appreciate and also would like to reflect that you actually discovered the issue.

dreamorosi avatar Aug 25 '22 10:08 dreamorosi

@rreddypally: In the meanwhile, if you don't want to use esbuild, you can add @middy/core as a dev dependency to your project (npm i -D @middy/core) and the build will succeed. I have verified that there's no trace of Middy in the built output so it won't have any effect on your end result.

I know it's not ideal but it's a temporary solution while we find a better one.

dreamorosi avatar Aug 25 '22 10:08 dreamorosi

@dreamorosi thanks for taking time to recreate the issue.

I can work with the dev dependency in the meantime as we use esbuild to bundle the code for deployment.

I have created issues for Logger and Metrics as requested. https://github.com/awslabs/aws-lambda-powertools-typescript/issues/1080 https://github.com/awslabs/aws-lambda-powertools-typescript/issues/1081

rreddypally avatar Aug 28 '22 09:08 rreddypally

Thank you @rreddypally, just to be clear, if you're using esbuild you shouldn't need the dev dependency at all. As far as we have see this is a problem only with tsc.

Thank you also for opening the issues, we'll discuss them with the team and report here any development

dreamorosi avatar Aug 28 '22 10:08 dreamorosi

We use tsc to compile the code locally and use esbuild to package the codebase for deployment. So we are facing the issue when working locally, which I can get around with dev dependencies as you mentioned.

rreddypally avatar Aug 29 '22 00:08 rreddypally

Thanks again @rreddypally for reporting this bug and providing a repo to reproduce the issue.

Upon a bit of investigation playing around with the repo ~~I found out why the compiler is complaining when folks are not importing the middleware file in their codebases~~ I came to a similar conclusion reached in a PR that was opened before creating this issue.
This happens because in the Tracer's declaration file located in ./node_modules/@aws-lambda-powertools/tracer/lib/Tracer.d.ts we are importing the TracerInterface via a wildcard import, instead of importing the TracerInterface file specifically. As a consequence, all the content of ./node_modules/@aws-lambda-powertools/tracer/lib/index.d.ts is loaded and checked by the compiler:

export * from './helpers';
export * from './Tracer';
export * from './TracerInterface';
export * from './middleware/middy';
//# sourceMappingURL=index.d.ts.map

Notice the 4th line, which is causing an error.

Steps to reproduce:

  • Clone the repo: https://github.com/rreddypally/tracer-demo
  • Run head -2 ./node_modules/@aws-lambda-powertools/tracer/lib/Tracer.d.ts and notice the TracerInterface import
  • Run npm run build and notice the error thrown
  • In your local codebase, edit the file ./node_modules/@aws-lambda-powertools/tracer/lib/Tracer.d.ts from this: import { TracerInterface } from '.'; to this: import { TracerInterface } from './TracerInterface';
  • Run npm run build and notice the lack of errors;
Screenshot 2022-09-08 at 14 29 56

The fix for this issue is make sure that the middy middleware file is not imported elsewhere like other folders and files, by removing it from example from the index file imports. Applicable to all other packages too.

saragerion avatar Sep 08 '22 12:09 saragerion

Hi "AWS Lambda Powertools Team", We are going to use Tracer, Logger, and Metrics for observability on our project and we are building our codebase with "tsc". Middy is still under consideration, so it would be nice to avoid redundant dependency on the extra library we don't use. Could you please share updates related to these 3 issues #1080, #1081, #1068, and when it's planned to be fixed?

ValeryShvyndzikau avatar Jan 10 '23 12:01 ValeryShvyndzikau

Hi @ValeryShvyndzikau, that's great to hear you'll be adopting the utilities. I just want to acknowledge that I read your message, I will be talking about this issue with @dreamorosi this week and we'll get back to you with a more concrete answer by the end of the week.

saragerion avatar Jan 10 '23 15:01 saragerion

To build on Sara's previous comment, currently the packages are exported like so:

Note From here onwards I talk about Tracer but the same applies to Metrics and Logger

File packages/tracer/src/index.ts

export * from './helpers';
export * from './Tracer';
export * from './TracerInterface';
export * from './middleware/middy';

which once built and packaged allows consumers to import like:

import { Tracer, captureLambdaHandler } from "@aws-lambda-powertools/tracer";

or, if not using the Middy middleware:

import { Tracer } from "@aws-lambda-powertools/tracer";

As reported by the discussion in this thread, when bundling with tsc the compiler throws an error because it expects @middy/core as dependency. This doesn't happen when using esbuild.

In order to fix this, it seems that we need to change the main entry point like so:

export * from './helpers';
export * from './Tracer';
export * from './TracerInterface';
-- export * from './middleware/middy';

This would allow everyone to still import the main Tracer class like before:

import { Tracer } from "@aws-lambda-powertools/tracer";

but, most notably, it'll require those who use the middleware to change their imports to:

import { captureLambdaHandler } from "@aws-lambda-powertools/tracer/middleware";

Now, this would solve the issue in question, but it would also constitute a breaking change, which will require us to do a major release to v2.0.0. We need to discuss how this fits with our roadmap.

dreamorosi avatar Jan 10 '23 17:01 dreamorosi

Yes I thought of the same solution as well. But I see some problem when you have installed the package and you do the proposed import

import { captureLambdaHandler } from "@aws-lambda-powertools/tracer/middleware";

The build will still fail as @middy/core is not installed locally. As esbuild removes all type checks, well the build works because @middy/core only contains type definitions. Once that would change even esbuild builds will crash at runtime. I would propose to create a new workspace @aws-powertools-lambda/middy or for each of the workspaces individually. Because that makes the used dependencies clear and does not have a risk of failing to run.

obiwabrakenobi avatar Jan 11 '23 10:01 obiwabrakenobi

I have tested the solution I described above (i.e. removing the export from packages/tracer/src/index.ts) in a custom build and installed it in the sample repo provided in one of the messages above. Then I built with tsc without installing @middy/core:

https://user-images.githubusercontent.com/7353869/211795347-9e0f63d0-0dfd-40c5-8844-40f51136bae6.mp4

As the video shows, when importing only import { Tracer } from '@aws-lambda-powertools/tracer'; it works. When instead importing the middleware import { captureLambdaHandler } from "@aws-lambda-powertools/tracer/middleware"; it gives a compile error.

It seems that by modifying the imports like suggested, when importing only from @aws-lambda-powertools/tracer, the contents of the middleware/* folder are never imported, removing the issue. However I agree that this is just a sample testing and further investigation is needed.

I'd like to avoid creating other dedicated packages only for the middleware utilities unless there's no other way that we can make this work without. Mainly for the reason that maintaining and distributing these packages will result in a significant overhead.

dreamorosi avatar Jan 11 '23 11:01 dreamorosi

Thanks to @saragerion, we have a workaround that we feel comfortable releasing in the short term, that doesn't require breaking changes in the imports, and that fixes the issue described above.

You can find more details in the linked PR #1225. If all goes well during review we should be able to make it available in the next release.

Thanks to everyone who contributed to the discussion.

dreamorosi avatar Jan 11 '23 17:01 dreamorosi

⚠️ 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.

github-actions[bot] avatar Jan 13 '23 09:01 github-actions[bot]

We just released version 1.5.1 that should fix the issue.

You can read more about the fix in #1225, however now you should be able to transpile your functions with tsc without having @middy/core in your codebase.

I have tested v1.5.1. in the repository provided here, and the code now compiles successfully to this:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const tracer_1 = require("@aws-lambda-powertools/tracer");
// import { captureLambdaHandler } from '@aws-lambda-powertools/tracer'
const tracer = new tracer_1.Tracer();

Hope this helps!

dreamorosi avatar Jan 13 '23 12:01 dreamorosi