opentelemetry-js
opentelemetry-js copied to clipboard
Change / Ban all usages of `export * from ...` so that all exports (and imports) are explicit
Make all imports and exports explicit so that internal classes / objects etc are not inadvertently made public and become part of the public API.
Would also recommend doing this, we are facing several issues from these type of imports.
I'd had a start at scripting this conversion back at https://github.com/open-telemetry/opentelemetry-js/issues/3976#issuecomment-1640496937 I could revive that to possibly help with a PR for this.
This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.
This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.
@dyladan I undid some of the notes you put into the issue title for contribfest.
I have been thinking about how to best go about this in SDK 2.0 from a practical standpoint and I think we should do this as much as we can on main first. I've noticed some caveats when merging main into next.
When a new export is added on main it will often be caught by the export * there. In next we then implicitly drop this new feature when updating as we do not get a merge-conflict when merging main into next.
My proposal: experienced contributors can do this wherever possible on main (one package at a time). That being said, we'll need to be very careful with reviewing.
This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.
To add more clarity on the expectations of this issue, here is my understanding of what should be done:
- Change all
export * from 'foo';toexport { foo } from 'foo';for parity - Add the ESLint rule
"no-restricted-syntax": ["error", "ExportAllDeclaration"],to error on these types of exports - Review the public API surface area to determine what should not be exported, what should be private, etc. This step is part of another issue and is considered out of scope for this particular issue.
Step 1 may be a single PR for the repo, or several smaller ones, which we are working through. Step 2 will be done as part of that single PR, or after the changes have been made. Step 3 is tracked separately in #3598
Packages remaining to be updated in otel-js core repo after #4880 merges:
- [ ] experimental/packages/exporter-logs-otlp-grpc/src/index.ts
- [ ] experimental/packages/exporter-trace-otlp-grpc/src/index.ts
- [ ] experimental/packages/exporter-trace-otlp-http/src/index.ts
- [ ] experimental/packages/exporter-trace-otlp-http/src/platform/browser/index.ts
- [ ] experimental/packages/exporter-trace-otlp-http/src/platform/index.ts
- [ ] experimental/packages/exporter-trace-otlp-http/src/platform/node/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/src/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/platform/browser/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/platform/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/platform/node/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-proto/src/index.ts
- [ ] experimental/packages/opentelemetry-sdk-node/src/index.ts
- [ ] experimental/packages/otlp-exporter-base/src/index.ts
@JamieDanielson @dyladan if required I can pick this up.
@JamieDanielson @dyladan if required I can pick this up.
Thanks! We have actually now made the majority of changes for this particular issue, and are intentionally holding off on otlp exporter changes for a bit because of other refactoring currently underway with this issue. I should have written that comment when I last updated this issue and forgot. That said, if you see another usage in this repo of export * from ... that were not handled or commented upon in #4880 please feel free to open a PR with that change!
no problem. I thought @robbkidd might be busy with something else hence was unassigned. thanks.