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

fix(instrumentation): do not export Node.js types for Browser users #4378

Closed
wants to merge 4 commits into from
Closed

fix(instrumentation): do not export Node.js types for Browser users #4378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b2da7f1
fix(instrumentation): do not export Node.js types for Browser users
pichlermarc Dec 18, 2023
d0d2c07
fix(changelog): add changelog entry
pichlermarc Dec 18, 2023
83ab4db
fix(instrumentation): undo import change to test file
pichlermarc Dec 18, 2023
acd8fd8
Merge branch 'main' into fix/instrumentation-node-types
pichlermarc Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ All notable changes to experimental packages in this project will be documented

### :bug: (Bug Fix)

* fix(instrumentation): do not export Node.js-specific types for Browser users [#4378](https://github.com/open-telemetry/opentelemetry-js/pull/4378) @pichlermarc
* Fixes a bug where `InstrumentationNodeModuleFile` and `InstrumentationNodeModuleDefinition` were exported even if using the package in a browser context

### :books: (Refine Doc)

### :house: (Internal)
Expand Down
5 changes: 2 additions & 3 deletions experimental/packages/opentelemetry-instrumentation/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@
*/

export * from './autoLoader';
export { InstrumentationBase } from './platform/index';
export { InstrumentationNodeModuleDefinition } from './instrumentationNodeModuleDefinition';
export { InstrumentationNodeModuleFile } from './instrumentationNodeModuleFile';
// Exports for Node.js and Browser are different, therefore exporting *
export * from './platform/index';
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

This is not the way the platform-specific exports work and will cause things to break

Copy link
Member Author

@pichlermarc pichlermarc Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Edit: some background info first: the problem is that we use path.normalize in InstrumentationNodeModuleDefinition, which does not exist in the browser.

@dyladan I see. We actually rely on this (different exports for different platforms) quite a bit in other packages in this repo (exporter packages, mostly), so we should likely also get on top of this there. It is also (in spirit) the way we had it before it was before we merged #4336.

From what I can tell there are only three other ways for us to fix this then:

  • provide and maintain our own normalize implementation
  • rely on a third-party package that provides an agnostic implementation
  • normalize in Node.js while using a no-op in the browser (this does not seem like a safe option)

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Before #4336 we actually had the bad situation I'm trying to avoid and I had to fix it to get that PR to pass.

normalize in Node.js while using a no-op in the browser (this does not seem like a safe option)

Why do you think this is unsafe? There is no valid reason to use a NodeModuleFile in browser anyway and it would be completely broken in other ways. The only thing we need to fix is that we shouldn't import path in browser contexts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking closer into this, I agree that it's likely fine if we do that (no-op in browser). Closing this PR, will re-open another one with that approach 👍

export * from './types';
export * from './types_internal';
export * from './utils';
6 changes: 5 additions & 1 deletion experimental/packages/opentelemetry-instrumentation/src/platform/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,8 @@
* limitations under the License.
*/

export { InstrumentationBase } from './node';
export {
InstrumentationNodeModuleFile,
InstrumentationNodeModuleDefinition,
InstrumentationBase,
} from './node';
2 changes: 2 additions & 0 deletions experimental/packages/opentelemetry-instrumentation/src/platform/node/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@
* limitations under the License.
*/
export { InstrumentationBase } from './instrumentation';
export { InstrumentationNodeModuleFile } from './instrumentationNodeModuleFile';
export { InstrumentationNodeModuleDefinition } from './instrumentationNodeModuleDefinition';
Comment on lines +17 to +18
Copy link
Member

Choose a reason for hiding this comment

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

The platform folder should not have any exports for 1 platform but not another. All exports should be used exactly identically. The same code that uses the node platform exports will also use the browser platform exports.

2 changes: 1 addition & 1 deletion ...rc/instrumentationNodeModuleDefinition.ts → ...de/instrumentationNodeModuleDefinition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import {
InstrumentationModuleDefinition,
InstrumentationModuleFile,
} from './types';
} from '../../types';

export class InstrumentationNodeModuleDefinition<T>
implements InstrumentationModuleDefinition<T>
Expand Down
2 changes: 1 addition & 1 deletion ...tion/src/instrumentationNodeModuleFile.ts → ...orm/node/instrumentationNodeModuleFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { InstrumentationModuleFile } from './types';
import { InstrumentationModuleFile } from '../../types';
import { normalize } from 'path';

export class InstrumentationNodeModuleFile<T>
Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import * as assert from 'assert';
import {
InstrumentationBase,
InstrumentationNodeModuleDefinition,
} from '../../build/src/index.js';
} from '../../build/src/platform/index.js';
import * as exported from 'test-esm-module';

class TestInstrumentationWrapFn extends InstrumentationBase {
Expand Down