opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

Change / Ban all usages of `export * from ...` so that all exports (and imports) are explicit

Open MSNev opened this issue 2 years ago • 11 comments

Make all imports and exports explicit so that internal classes / objects etc are not inadvertently made public and become part of the public API.

MSNev avatar Oct 04 '23 16:10 MSNev

Would also recommend doing this, we are facing several issues from these type of imports.

jdbadilla avatar Oct 20 '23 15:10 jdbadilla

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.

trentm avatar Oct 25 '23 17:10 trentm

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.

github-actions[bot] avatar Feb 05 '24 06:02 github-actions[bot]

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.

github-actions[bot] avatar Apr 15 '24 06:04 github-actions[bot]

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

pichlermarc avatar Apr 16 '24 09:04 pichlermarc

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.

github-actions[bot] avatar Jul 01 '24 06:07 github-actions[bot]

To add more clarity on the expectations of this issue, here is my understanding of what should be done:

  1. Change all export * from 'foo'; to export { foo } from 'foo'; for parity
  2. Add the ESLint rule "no-restricted-syntax": ["error", "ExportAllDeclaration"], to error on these types of exports
  3. 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

JamieDanielson avatar Jul 26 '24 16:07 JamieDanielson

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 avatar Aug 02 '24 18:08 JamieDanielson

@JamieDanielson @dyladan if required I can pick this up.

legalimpurity avatar Aug 14 '24 19:08 legalimpurity

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

JamieDanielson avatar Aug 15 '24 20:08 JamieDanielson

no problem. I thought @robbkidd might be busy with something else hence was unassigned. thanks.

legalimpurity avatar Aug 16 '24 07:08 legalimpurity