-
Notifications
You must be signed in to change notification settings - Fork 256
[Beta] Merge Common into Core #2693
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request merges the @microsoft/applicationinsights-common package into @microsoft/applicationinsights-core-js as part of preparing for alignment with the OTel-SDK branch. This consolidation simplifies the dependency tree and improves tree-shaking capabilities.
Changes:
- Merged all Common package source code into Core under
src/Common/directory - Created a compatibility layer in the Common package that re-exports from Core for backward compatibility
- Updated all internal imports in Common source files to reference Core's JavaScriptSDK modules
- Updated test files and moved Common tests into Core's test suite
- Added comprehensive documentation including migration guide and updated READMEs
Reviewed changes
Copilot reviewed 54 out of 114 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shared/AppInsightsCore/tsconfig.json | Added Common source directories to TypeScript compilation |
| shared/AppInsightsCore/src/applicationinsights-core-js.ts | Added comprehensive exports for all Common package functionality |
| shared/AppInsightsCore/src/OpenTelemetry/**/*.ts | Minor whitespace/formatting fixes (removed double semicolons, trailing spaces) |
| shared/AppInsightsCore/src/Common/**/*.ts | All Common source files moved to Core with updated import paths |
| shared/AppInsightsCore/Tests/Unit/src/**/*.ts | Updated test imports and added Common tests to Core test suite |
| shared/AppInsightsCore/README.md | Added deprecation notice and migration information |
| shared/AppInsightsCommon/src/applicationinsights-common.ts | Replaced with compatibility layer that re-exports from Core |
| shared/AppInsightsCommon/package.json | Disabled test scripts (tests now run in Core) |
| shared/AppInsightsCommon/Tests/** | Removed test files and configurations |
| shared/AppInsightsCommon/README.md | Added deprecation warning with migration instructions |
| docs/upgrade/MergeCommonToCore.md | Comprehensive migration guide for users |
| RELEASES.md | Documented the merge with timeline for deprecation |
| gruntfile.js | Disabled Common package test tasks |
| .aiAutoMinify.json | Moved const enums from Common to Core configuration |
| channels/applicationinsights-channel-js/** | Updated imports to reference Core instead of Common |
| AISKU/Tests/**/*.ts | Updated imports to reference Core instead of Common |
… into Core - Update imports in Common telemetry files to use relative paths instead of external package references - Add comprehensive exports from applicationinsights-core-js.ts for all Common module functionality including telemetry classes, interfaces, enums, helper functions, contracts, and plugin identifiers
f5ac5ff to
e60c0ce
Compare
a1d181e to
26b8d7b
Compare
54681c6 to
ba5e3c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Phase 3: Add/Adjust tree-shaking hints and reduce duplication from the merge Phase 4: Stop using applicationinsights-common (missed by Co-Pilot)
ba5e3c5 to
7054b1b
Compare
| @@ -0,0 +1,1438 @@ | |||
| # Move Layout Plan: AppInsightsCore → otel-core Structure | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be removed right?
| @@ -0,0 +1,2160 @@ | |||
| # AppInsightsCommon → AppInsightsCore Merge Plan | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this as well
rads-1996
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As part of preparing for merging changes between OTel-SDK branch and Beta merging Common into Core.
Next Steps: restructuring the folder structure for Core to align with the OTel-SDK
otel-core-jsproject