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.

Sign up for GitHub

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

Jump to bottom

feat: added native esm support #1159

Merged
merged 19 commits into from
Jun 25, 2024
Merged

feat: added native esm support #1159

merged 19 commits into from
Jun 25, 2024

Conversation

aryamohanan
Copy link
Contributor

@aryamohanan aryamohanan commented May 22, 2024 •
edited
Loading

Support native ESM modules using IITM
refs INSTA-807

Tasks:

  • Find a way to test native esm without got or native-fetch.
  • Add test cases to verify the functionality of native ESM modules.
  • Resolve issue with multiple instance of iitm module in local
  • 100% ensure we avoid duplicates (add strong tests) -> there are many tests who do not check the length of the spans (only db2) - maybe double check to add some more assertions to these tests (handful)
  • Resolve any issues in existing test cases.
  • Run pipeline to test for All ESM supported versions are passing before merge

Cases

The import-in-the-middle (IITM) package throws a "Cannot find module" error when importing the Prisma package from a test folder (basically running our prisma esm tests ) on Node.js version 18.19 and above. This issue does not occur if the Prisma client is installed in the root package.json or if the IITM hook is not used.
See nodejs/import-in-the-middle#97

@aryamohanan aryamohanan changed the title feat(native-esm): added native esm support for collector with iitm feat(native-esm): added native esm support with import-in-the-middle May 22, 2024
@aryamohanan aryamohanan force-pushed the native-esm-support branch 2 times, most recently from e810212 to 36bfa8d Compare May 28, 2024 06:54
@aryamohanan aryamohanan force-pushed the native-esm-support branch 2 times, most recently from f04d8f9 to 9431241 Compare May 30, 2024 16:50
@aryamohanan aryamohanan requested review from kirrg001 June 3, 2024 12:50
@aryamohanan aryamohanan force-pushed the native-esm-support branch 2 times, most recently from 5a39f51 to 0da88e8 Compare June 10, 2024 14:20
@aryamohanan aryamohanan marked this pull request as ready for review June 12, 2024 12:43
@aryamohanan aryamohanan requested a review from a team as a code owner June 12, 2024 12:43
kirrg001
kirrg001 reviewed Jun 13, 2024
View reviewed changes
Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some first questions :)

packages/collector/test/tracing/cloud/aws-sdk/v2/dynamodb/test.js Outdated Show resolved Hide resolved
packages/core/src/util/iitmHook.js Show resolved Hide resolved
packages/google-cloud-run/esm-loader.mjs Show resolved Hide resolved
packages/core/src/util/applicationUnderMonitoring.js Show resolved Hide resolved
packages/core/package.json Outdated Show resolved Hide resolved
packages/core/src/tracing/index.js Outdated Show resolved Hide resolved
@aryamohanan aryamohanan mentioned this pull request Jun 17, 2024
Merged
@aryamohanan aryamohanan removed the WIP label Jun 17, 2024
kirrg001
kirrg001 reviewed Jun 18, 2024
View reviewed changes
packages/core/src/util/esm.js Outdated
(process.execArgv[0].indexOf('--import') !== -1 && process.execArgv[1].indexOf('esm-register.mjs') !== -1)));

// @ts-ignore
require.cache[cacheKey] = { exports: isESM };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require.cache is a file cache. Please do not abuse it for caching a local variable.

kirrg001
kirrg001 reviewed Jun 18, 2024
View reviewed changes
Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some more questions

packages/collector/test/tracing/native_esm/app.mjs Outdated Show resolved Hide resolved
packages/aws-fargate/esm-loader.mjs Outdated Show resolved Hide resolved
packages/core/src/util/iitmHook.js Outdated Show resolved Hide resolved
packages/core/src/util/iitmHook.js Show resolved Hide resolved
@aryamohanan aryamohanan requested a review from kirrg001 June 20, 2024 10:16
@kirrg001
Copy link
Contributor

I will merge & approve this til Monday. We will release it next week

kirrg001
kirrg001 requested changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested two more chances

packages/core/src/util/hook.js Outdated
*
* This array lists all the libraries that are native ECMAScript Modules.
*/
const pureEsmLibraries = ['square-calc'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be changed as well. This is no longer correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best approach I guess is:

hook.onModuleLoad('square-calc', { nativeEsm: true})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the code

packages/core/src/util/hook.js Outdated Show resolved Hide resolved
@aryamohanan aryamohanan requested a review from kirrg001 June 25, 2024 03:51
kirrg001
kirrg001 approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

@kirrg001 kirrg001 merged commit 8bfef61 into main Jun 25, 2024
4 checks passed
@kirrg001 kirrg001 deleted the native-esm-support branch June 25, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirrg001 kirrg001 kirrg001 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aryamohanan @kirrg001