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

Jump to bottom

refactor: remove "export *" in favor of explicit named exports #4880

Merged
merged 15 commits into from
Aug 9, 2024
Merged

refactor: remove "export *" in favor of explicit named exports #4880

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0ba7e65
add no export-star rule to api's eslint
robbkidd Jul 25, 2024
919854d
no more export-star, only specifics
robbkidd Jul 25, 2024
e1aab56
declare the rule at the root
robbkidd Jul 26, 2024
dd73e23
add baggageUtils check to integration tests
robbkidd Jul 30, 2024
45accb1
make implicit re-exports explicit
robbkidd Jul 30, 2024
6b46bd8
dial eslint down to warn on exports being worked separately
robbkidd Jul 31, 2024
8a4b892
Merge branch 'main' into api.no-export-star
robbkidd Jul 31, 2024
0fe69ad
disable warning about 'export *' in semconv
robbkidd Aug 1, 2024
f0124ea
move assertions about core to separate test file
robbkidd Aug 7, 2024
fbe20c5
Merge remote-tracking branch 'upstream/main' into api.no-export-star
robbkidd Aug 7, 2024
9c0b671
remove ts-node from integration test deps
robbkidd Aug 7, 2024
fbe156f
update changelogs
robbkidd Aug 7, 2024
d0a2b85
omit listing pkgs that only had doc comments added
robbkidd Aug 8, 2024
afcbcc4
Update CHANGELOG.md
pichlermarc Aug 9, 2024
a34c7b7
Merge branch 'main' into api.no-export-star
pichlermarc Aug 9, 2024
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
2 changes: 1 addition & 1 deletion api/src/platform/browser/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './globalThis';
export { _globalThis } from './globalThis';
2 changes: 1 addition & 1 deletion api/src/platform/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './node';
export { _globalThis } from './node';
2 changes: 1 addition & 1 deletion api/src/platform/node/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './globalThis';
export { _globalThis } from './globalThis';
1 change: 1 addition & 0 deletions eslint.base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ module.exports = {
}
}],
"@typescript-eslint/no-shadow": ["warn"],
"no-restricted-syntax": ["error", "ExportAllDeclaration"],
"prefer-rest-params": "off",
}
},
Expand Down
8 changes: 4 additions & 4 deletions experimental/packages/api-events/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
* limitations under the License.
*/

export * from './types/EventLogger';
export * from './types/EventLoggerProvider';
export * from './types/Event';
export * from './types/EventLoggerOptions';
export { EventLogger } from './types/EventLogger';
export { EventLoggerProvider } from './types/EventLoggerProvider';
export { Event } from './types/Event';
export { EventLoggerOptions } from './types/EventLoggerOptions';

import { EventsAPI } from './api/events';
export const events = EventsAPI.getInstance();
2 changes: 1 addition & 1 deletion experimental/packages/api-events/src/platform/browser/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './globalThis';
export { _globalThis } from './globalThis';
2 changes: 1 addition & 1 deletion experimental/packages/api-events/src/platform/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './node';
export { _globalThis } from './node';
2 changes: 1 addition & 1 deletion experimental/packages/api-events/src/platform/node/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './globalThis';
export { _globalThis } from './globalThis';
19 changes: 12 additions & 7 deletions experimental/packages/api-logs/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,18 @@
* limitations under the License.
*/

export * from './types/Logger';
export * from './types/LoggerProvider';
export * from './types/LogRecord';
export * from './types/LoggerOptions';
export * from './types/AnyValue';
export * from './NoopLogger';
export * from './NoopLoggerProvider';
export { Logger } from './types/Logger';
export { LoggerProvider } from './types/LoggerProvider';
export {
LogAttributes,
LogBody,
LogRecord,
SeverityNumber,
} from './types/LogRecord';
export { LoggerOptions } from './types/LoggerOptions';
export { AnyValue, AnyValueMap } from './types/AnyValue';
export { NOOP_LOGGER, NoopLogger } from './NoopLogger';
export { NOOP_LOGGER_PROVIDER, NoopLoggerProvider } from './NoopLoggerProvider';

import { LogsAPI } from './api/logs';
export const logs = LogsAPI.getInstance();
2 changes: 1 addition & 1 deletion experimental/packages/api-logs/src/platform/browser/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './globalThis';
export { _globalThis } from './globalThis';
2 changes: 1 addition & 1 deletion experimental/packages/api-logs/src/platform/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './node';
export { _globalThis } from './node';
2 changes: 1 addition & 1 deletion experimental/packages/api-logs/src/platform/node/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './globalThis';
export { _globalThis } from './globalThis';
3 changes: 3 additions & 0 deletions experimental/packages/exporter-logs-otlp-grpc/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './OTLPLogExporter';
3 changes: 3 additions & 0 deletions experimental/packages/exporter-trace-otlp-grpc/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './OTLPTraceExporter';
3 changes: 3 additions & 0 deletions experimental/packages/exporter-trace-otlp-http/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './platform';
3 changes: 3 additions & 0 deletions experimental/packages/exporter-trace-otlp-http/src/platform/browser/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './OTLPTraceExporter';
3 changes: 3 additions & 0 deletions experimental/packages/exporter-trace-otlp-http/src/platform/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './node';
3 changes: 3 additions & 0 deletions experimental/packages/exporter-trace-otlp-http/src/platform/node/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './OTLPTraceExporter';
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-browser-detector/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export * from './BrowserDetector';
export { browserDetector } from './BrowserDetector';
3 changes: 3 additions & 0 deletions experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './OTLPMetricExporter';
3 changes: 3 additions & 0 deletions experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './platform';
export * from './OTLPMetricExporterOptions';
export * from './OTLPMetricExporterBase';
3 changes: 3 additions & 0 deletions experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/platform/browser/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './OTLPMetricExporter';
3 changes: 3 additions & 0 deletions experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/platform/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './node';
3 changes: 3 additions & 0 deletions experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/platform/node/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './OTLPMetricExporter';
3 changes: 3 additions & 0 deletions experimental/packages/opentelemetry-exporter-metrics-otlp-proto/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './OTLPMetricExporter';
6 changes: 3 additions & 3 deletions experimental/packages/opentelemetry-exporter-prometheus/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@
* limitations under the License.
*/

export * from './PrometheusExporter';
export * from './PrometheusSerializer';
export * from './export/types';
export { PrometheusExporter } from './PrometheusExporter';
export { PrometheusSerializer } from './PrometheusSerializer';
export { ExporterConfig } from './export/types';
6 changes: 5 additions & 1 deletion experimental/packages/opentelemetry-instrumentation-fetch/src/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 * from './fetch';
export {
FetchCustomAttributeFunction,
FetchInstrumentation,
FetchInstrumentationConfig,
} from './fetch';
49 changes: 46 additions & 3 deletions experimental/packages/opentelemetry-instrumentation-http/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,49 @@
* limitations under the License.
*/

export * from './http';
export * from './types';
export * from './utils';
export { HttpInstrumentation } from './http';
export {
Err,
Func,
GetFunction,
Http,
HttpCallback,
HttpCallbackOptional,
HttpCustomAttributeFunction,
HttpInstrumentationConfig,
HttpRequestArgs,
HttpRequestCustomAttributeFunction,
HttpResponseCustomAttributeFunction,
Https,
IgnoreIncomingRequestFunction,
IgnoreMatcher,
IgnoreOutgoingRequestFunction,
ParsedRequestOptions,
RequestFunction,
RequestSignature,
StartIncomingSpanCustomAttributeFunction,
StartOutgoingSpanCustomAttributeFunction,
} from './types';
export {
extractHostnameAndPort,
getAbsoluteUrl,
getIncomingRequestAttributes,
getIncomingRequestAttributesOnResponse,
getIncomingRequestMetricAttributes,
getIncomingRequestMetricAttributesOnResponse,
getOutgoingRequestAttributes,
getOutgoingRequestAttributesOnResponse,
getOutgoingRequestMetricAttributes,
getOutgoingRequestMetricAttributesOnResponse,
getRequestInfo,
headerCapture,
isCompressed,
isIgnored,
isValidOptionsType,
parseResponseStatus,
satisfiesPattern,
setAttributesFromHttpKind,
setRequestContentLengthAttribute,
setResponseContentLengthAttribute,
setSpanWithError,
} from './utils';
6 changes: 5 additions & 1 deletion experimental/packages/opentelemetry-instrumentation-xml-http-request/src/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 * from './xhr';
export {
XHRCustomAttributeFunction,
XMLHttpRequestInstrumentation,
XMLHttpRequestInstrumentationConfig,
} from './xhr';
17 changes: 14 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 @@ -18,6 +18,17 @@ export { registerInstrumentations } from './autoLoader';
export { InstrumentationBase } from './platform/index';
export { InstrumentationNodeModuleDefinition } from './instrumentationNodeModuleDefinition';
export { InstrumentationNodeModuleFile } from './instrumentationNodeModuleFile';
export * from './types';
export * from './types_internal';
export * from './utils';
export {
Instrumentation,
InstrumentationConfig,
InstrumentationModuleDefinition,
InstrumentationModuleFile,
ShimWrapped,
SpanCustomizationHook,
} from './types';
export { AutoLoaderOptions, AutoLoaderResult } from './types_internal';
export {
isWrapped,
safeExecuteInTheMiddle,
safeExecuteInTheMiddleAsync,
} from './utils';
7 changes: 5 additions & 2 deletions experimental/packages/opentelemetry-sdk-node/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * as api from '@opentelemetry/api';
export * as contextBase from '@opentelemetry/api';
export * as core from '@opentelemetry/core';
Expand All @@ -22,5 +25,5 @@ export * as metrics from '@opentelemetry/sdk-metrics';
export * as node from '@opentelemetry/sdk-trace-node';
export * as resources from '@opentelemetry/resources';
export * as tracing from '@opentelemetry/sdk-trace-base';
export * from './sdk';
export * from './types';
export { LoggerProviderConfig, MeterProviderConfig, NodeSDK } from './sdk';
export { NodeSDKConfiguration } from './types';
4 changes: 4 additions & 0 deletions experimental/packages/otlp-exporter-base/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/* eslint no-restricted-syntax: ["warn", "ExportAllDeclaration"] --
* TODO: Replace export * with named exports before next major version
*/
export * from './platform';
export { OTLPExporterBase } from './OTLPExporterBase';
export {
Expand Down
4 changes: 3 additions & 1 deletion integration-tests/api/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,14 @@
},
"devDependencies": {
"@opentelemetry/api": "^1.0.0",
"@opentelemetry/core": "^1.0.0",
"@types/mocha": "10.0.7",
"@types/node": "18.6.5",
"codecov": "3.8.3",
"cross-var": "1.1.0",
"lerna": "6.6.2",
"mocha": "10.0.0",
"nyc": "15.1.0"
"nyc": "15.1.0",
"ts-node": "^10.9.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why this ts-node devDep was added?

Copy link
Member

Choose a reason for hiding this comment

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

🤔 I wonder if we left that in when trying out some other integration tests. Doesn't look like we ended up using it for anything, should be okay to drop.

Copy link
Member

Choose a reason for hiding this comment

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

I think the way we've set it up now will always require ts-node when we're using mocha, even if we're just feeding it JavaScript code (see ./.mocharc.yml, which is the closes mocha config file).

So I think we'd actually need to add this ts-node dependency here.

Copy link
Member Author

@robbkidd robbkidd Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you know why this ts-node devDep was added?

… I did last week.

I removed ts-node from package.json and the root package-lock.json, rm'd root ./node_modules, ran npm run reset, then this integration test cd integration-tests/api && npm run test, and the tests ran and passed. So … I can't explain the dev need for ts-node any more.

Update: I removed ts-node from the devDeps and the test suite seems fine.

Copy link
Member Author

@robbkidd robbkidd Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding changelog entries: should supply multiple entries to a CHANGELOG (one entry for every package touched in the closest parent CHANGELOG)? I would model each entry's content on this past exports refactoring.

Changelogs! 
❯ find ./ -type d -name node_modules -prune -o -name CHANGELOG.md -print | sort
.//CHANGELOG.md
.//api/CHANGELOG.md
.//experimental/CHANGELOG.md
.//experimental/packages/otlp-transformer/protos/CHANGELOG.md
.//scripts/semconv/opentelemetry-specification/CHANGELOG.md

Copy link
Member

Choose a reason for hiding this comment

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

I think we should skip the changelog in the otlp-transformer package and the scripts directory, so we'd maybe want only in these:

  • .//CHANGELOG.md
  • .//api/CHANGELOG.md
  • .//experimental/CHANGELOG.md

Copy link
Member Author

Choose a reason for hiding this comment

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

Changelog entries added in fbe156f. I opted to list each package individually in sub-bullets because I didn't see prior art for very long lists of updated packages. I'm happy to adjust that content—flatten to a single bullet with a comma-separated list, put the long list in refactor(...):, remove the list—however y'all see fit.

Copy link
Contributor

@trentm trentm Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Update: I removed ts-node from the devDeps and the test suite seems fine.

I understand the ts-node dep now. Marc's comment is correct. In #4840 we switched from using ts-mocha to just using mocha directly with a config file (https://github.com/open-telemetry/opentelemetry-js/blob/main/.mocharc.yml) that uses ts-node (to handle the compilation of TS to JS). That means that technically any package.json file with a "mocha" devDep should also have a "ts-node" devDep. Because we are using npm workspaces with a shared top-level node_modules/... folder, any single package.json have a ts-node devDep that gets installed at the top-level saves us. And there are three:

% rg ts-node -g package.json
packages/opentelemetry-semantic-conventions/package.json
90:    "ts-node": "10.9.2",

experimental/examples/events/package.json
6:    "start": "ts-node index.ts"
18:    "ts-node": "^10.9.1"

experimental/examples/logs/package.json
6:    "start": "ts-node index.ts",
16:    "ts-node": "^10.9.1"

So, technically adding that devDep here was a good thing, but this is hardly the only transgressor. There are 3 package.json's with the "ts-node" dep and 44 with the "mocha" dep:

% rg '"mocha":' -g package.json | wc -l
      44

}
}
10 changes: 10 additions & 0 deletions integration-tests/api/test/api-entries.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,14 @@ describe('@opentelemetry/api entries', () => {
const mod = require('@opentelemetry/api/experimental');
assert.ok(mod.wrapTracer != null);
});

it('should import baggage utils', async () => {
const mod = await import('@opentelemetry/core');
assert.ok(mod.baggageUtils.getKeyPairs != null);
});

it('should require baggage utils', () => {
const mod = require('@opentelemetry/core');
assert.ok(mod.baggageUtils.getKeyPairs != null);
});
pichlermarc marked this conversation as resolved.
Show resolved Hide resolved
});
Loading
Loading