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.

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

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

Open
MSNev opened this issue Oct 4, 2023 · 12 comments

Comments

@MSNev
Copy link
Contributor

MSNev commented Oct 4, 2023

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 MSNev added the triage label Oct 4, 2023
@MSNev MSNev added this to the OpenTelemetry SDK 2.0 milestone Oct 4, 2023
@jdbadilla
Copy link

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

@dyladan dyladan added triage:accepted This feature has been accepted and removed triage labels Oct 25, 2023
@trentm
Copy link
Contributor

trentm commented Oct 25, 2023

I'd had a start at scripting this conversion back at #3976 (comment)
I could revive that to possibly help with a PR for this.

@dyladan dyladan added the contribfest These small and isolated issues are suitable for Kubecon Contribfest label Nov 8, 2023
Copy link

github-actions bot commented Feb 5, 2024

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 github-actions bot added the stale label Feb 5, 2024
@trentm trentm removed the stale label Feb 7, 2024
Copy link

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 github-actions bot added the stale label Apr 15, 2024
@pichlermarc pichlermarc added feature-request and removed stale contribfest These small and isolated issues are suitable for Kubecon Contribfest labels Apr 16, 2024
@pichlermarc
Copy link
Member

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

Copy link

github-actions bot commented Jul 1, 2024

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.

@JamieDanielson
Copy link
Member

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
Copy link
Member

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

@legalimpurity
Copy link
Contributor

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

@JamieDanielson
Copy link
Member

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

@legalimpurity
Copy link
Contributor

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

@JamieDanielson
Copy link
Member

Because of the eslint rule added for "no-restricted-syntax", every remaining instance of "export * from" was intentionally set disabled. Final step on this issue is to review those packages that contain eslint-disable no-restricted-syntax and determine if things need to change, and/or if a comment needs to be updated. For example:

  • Semantic conventions will likely stay as-is
  • OTLP exporters are under a major refactor, which will remove some of these already

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants