Skip to content

Bug: unable to build with tsc when not using @middy/core #1068

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

Closed
rreddypally opened this issue Aug 22, 2022 · 18 comments · Fixed by #1225
Closed

Bug: unable to build with tsc when not using @middy/core #1068

rreddypally opened this issue Aug 22, 2022 · 18 comments · Fixed by #1225
Assignees
Labels
bug Something isn't working completed This item is complete and has been merged/shipped tracer This item relates to the Tracer Utility

Comments

@rreddypally
Copy link

rreddypally commented Aug 22, 2022

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 rreddypally added bug Something isn't working triage This item has not been triaged by a maintainer, please wait labels Aug 22, 2022
rreddypally added a commit to rreddypally/aws-lambda-powertools-typescript that referenced this issue Aug 22, 2022
@dreamorosi
Copy link
Contributor

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).

2022-08-22.14-03-14.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 dreamorosi added the pending-close-response-required This issue will be closed soon unless the discussion moves forward label Aug 24, 2022
@rreddypally
Copy link
Author

rreddypally commented Aug 25, 2022

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.

@dreamorosi
Copy link
Contributor

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 dreamorosi removed the pending-close-response-required This issue will be closed soon unless the discussion moves forward label Aug 25, 2022
@rreddypally
Copy link
Author

No problem at all.

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

Let me know if you have trouble accessing it.

@dreamorosi
Copy link
Contributor

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 dreamorosi changed the title Bug (Tracer): Tracer imports all the components via TracerInterface Bug (Tracer): unable to build with tsc when not using @middy/core Aug 25, 2022
@dreamorosi
Copy link
Contributor

@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.

@rreddypally
Copy link
Author

@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.
#1080
#1081

@dreamorosi
Copy link
Contributor

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

@rreddypally
Copy link
Author

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.

@saragerion saragerion added the help-wanted We would really appreciate some support from community for this one label Sep 8, 2022
@saragerion
Copy link
Contributor

saragerion commented Sep 8, 2022

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.

@dreamorosi dreamorosi added the good-first-issue Something that is suitable for those who want to start contributing label Oct 27, 2022
@dreamorosi dreamorosi removed the triage This item has not been triaged by a maintainer, please wait label Nov 13, 2022
@dreamorosi dreamorosi added tracer This item relates to the Tracer Utility need-investigation need-more-information Requires more information before making any calls and removed area/tracer labels Nov 13, 2022
@dreamorosi dreamorosi added the blocked This item's progress is blocked by external dependency or reason label Nov 13, 2022
@dreamorosi dreamorosi changed the title Bug (Tracer): unable to build with tsc when not using @middy/core Bug: unable to build with tsc when not using @middy/core Nov 14, 2022
@ValeryShvyndzikau
Copy link

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?

@saragerion
Copy link
Contributor

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.

@dreamorosi
Copy link
Contributor

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.

@obiwabrakenobi
Copy link

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.

@dreamorosi
Copy link
Contributor

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:

2023-01-11.12-13-23.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 dreamorosi linked a pull request Jan 11, 2023 that will close this issue
13 tasks
@dreamorosi
Copy link
Contributor

dreamorosi commented Jan 11, 2023

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 dreamorosi added confirmed The scope is clear, ready for implementation and removed good-first-issue Something that is suitable for those who want to start contributing help-wanted We would really appreciate some support from community for this one blocked This item's progress is blocked by external dependency or reason need-more-information Requires more information before making any calls labels Jan 11, 2023
@dreamorosi dreamorosi self-assigned this Jan 11, 2023
@dreamorosi dreamorosi moved this from Backlog to Pending review in AWS Lambda Powertools for TypeScript Jan 11, 2023
@github-project-automation github-project-automation bot moved this from Pending review to Coming soon in AWS Lambda Powertools for TypeScript Jan 13, 2023
@github-actions
Copy link
Contributor

⚠️ 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 github-actions bot added the pending-release This item has been merged and will be released soon label Jan 13, 2023
@dreamorosi
Copy link
Contributor

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 dreamorosi added completed This item is complete and has been merged/shipped and removed pending-release This item has been merged and will be released soon confirmed The scope is clear, ready for implementation labels Jan 13, 2023
@dreamorosi dreamorosi moved this from Coming soon to Shipped in AWS Lambda Powertools for TypeScript Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working completed This item is complete and has been merged/shipped tracer This item relates to the Tracer Utility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants