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

[core-rest-pipeline] Add conditional exports #22804

Merged
merged 19 commits into from
Oct 20, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ee27d04
Conditional export for core-rest-pipeline
jeremymeng Jun 13, 2022
1c0c4fb
Add an option to dev-tool bundle command for .cjs output extension
jeremymeng Aug 1, 2022
db7e7ef
import "*" => import "*.js"
jeremymeng Aug 1, 2022
aa393d5
bump mocha version to ^10.0.0
jeremymeng Aug 2, 2022
31632f4
- import * as => import
jeremymeng Aug 4, 2022
db5ed1f
Merge remote-tracking branch 'up/main' into core/cond-export
jeremymeng Aug 4, 2022
d9fe9ee
- Fix linter rule to allow `"main": "dist/index.cjs"`
jeremymeng Aug 4, 2022
d874bc1
revert back to awesome example.com
jeremymeng Aug 4, 2022
65524e6
Fix types export
jeremymeng Aug 4, 2022
76a44b5
Merge remote-tracking branch 'up/main' into core/cond-export
jeremymeng Aug 4, 2022
5231081
Bump minor version and update CHANGELOG
jeremymeng Aug 4, 2022
279160c
Merge remote-tracking branch 'up/main' into core/cond-export
jeremymeng Aug 5, 2022
00d9c25
Merge remote-tracking branch 'up/main' into core/cond-export
jeremymeng Aug 29, 2022
6bcb49c
Merge remote-tracking branch 'up/main' into core/cond-export
jeremymeng Oct 5, 2022
10f3fa6
Add copy of our types shim for CommonJS conditional export
jeremymeng Oct 6, 2022
5bb5b13
remove `output-cjs-ext` option
jeremymeng Oct 17, 2022
ccc1365
Merge remote-tracking branch 'upstream/main' into core/cond-export
jeremymeng Oct 17, 2022
57705cf
delete usage of removed dev-tool bundle option
jeremymeng Oct 20, 2022
df32ab1
Merge remote-tracking branch 'up/main' into core/cond-export
jeremymeng Oct 20, 2022
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
13 changes: 8 additions & 5 deletions common/config/rush/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion common/tools/dev-tool/src/commands/run/bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ export const commandInfo = makeCommandInfo(
default: true,
description: "include a polyfill for Node.js builtin modules",
},
"output-cjs-ext": {
kind: "boolean",
default: false,
description: "use .cjs extension for bundle output. default is .js extension",
},
}
);

Expand Down Expand Up @@ -72,7 +77,7 @@ export default leafCommand(commandInfo, async (options) => {
const bundle = await rollup.rollup(baseConfig);

await bundle.write({
file: "dist/index.js",
file: `dist/index.${options["output-cjs-ext"] ? "cjs" : "js"}`,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using an option, could we just read the main path from package.json?

Suggested change
file: `dist/index.${options["output-cjs-ext"] ? "cjs" : "js"}`,
file: info.packageJson.main

It may be best to defensively check that this field is set and starts with dist just to make sure we don't blow up someone's machine from a nasty config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point! I changed to use the main path. We already have the linter to ensure the main path so I just did a minimal check for non-falsy values.

format: "cjs",
sourcemap: true,
exports: "named",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# ts-package-json-main-is-cjs

Requires `main` in `package.json` to be point to a CommonJS or UMD module. In this case, its assumed that it points to `"dist/index.js"`.
Requires `main` in `package.json` to be point to a CommonJS or UMD module. In this case, its assumed that it points to `"dist/index.js"` or `"dist/index.cjs"`.

This rule is fixable using the `--fix` option.

Expand All @@ -14,6 +14,14 @@ This rule is fixable using the `--fix` option.
}
```

### Good

```json
{
"main": "dist/index.cjs"
}
```

### Bad

```json
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ export = {
const nodeValue = node.value as Literal;
const main = nodeValue.value as string;

if (!/^(\.\/)?dist\/index\.js$/.test(main)) {
if (!/^(\.\/)?dist\/index\.(c)?js$/.test(main)) {
context.report({
node: nodeValue,
message: `main is set to ${main} when it should be set to dist/index.js`,
message: `main is set to ${main} when it should be set to dist/index.js or dist/index.cjs`,
fix: (fixer: Rule.RuleFixer): Rule.Fix =>
fixer.replaceText(nodeValue, `"dist/index.js"`),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,18 +286,17 @@ export = {
const symbol = typeChecker
.getTypeAtLocation(converter.get(node as TSESTree.Node))
.getSymbol();
const overloads =
symbol?.declarations
? symbol.declarations
.filter(
(declaration: Declaration): boolean =>
reverter.get(declaration as TSNode) !== undefined
)
.map((declaration: Declaration): FunctionExpression => {
const method = reverter.get(declaration as TSNode) as MethodDefinition;
return method.value;
})
: [];
const overloads = symbol?.declarations
? symbol.declarations
.filter(
(declaration: Declaration): boolean =>
reverter.get(declaration as TSNode) !== undefined
)
.map((declaration: Declaration): FunctionExpression => {
const method = reverter.get(declaration as TSNode) as MethodDefinition;
return method.value;
})
: [];
evaluateOverloads(
overloads,
converter,
Expand Down Expand Up @@ -326,13 +325,12 @@ export = {
const symbol = typeChecker
.getTypeAtLocation(converter.get(node as TSESTree.Node))
.getSymbol();
const overloads =
symbol?.declarations
? symbol.declarations.map(
(declaration: Declaration): FunctionDeclaration =>
reverter.get(declaration as TSNode) as FunctionDeclaration
)
: [];
const overloads = symbol?.declarations
? symbol.declarations.map(
(declaration: Declaration): FunctionDeclaration =>
reverter.get(declaration as TSNode) as FunctionDeclaration
)
: [];
evaluateOverloads(
overloads,
converter,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,16 @@ ruleTester.run("ts-package-json-main-is-cjs", rule, {
code: '{"main": "./dist/index.js"}',
filename: "package.json",
},
{
// correct format #1
code: '{"main": "dist/index.cjs"}',
filename: "package.json",
},
{
// correct format #2
code: '{"main": "./dist/index.cjs"}',
filename: "package.json",
},
{
// a full example package.json (taken from https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/eventhub/event-hubs/package.json with "scripts" removed for testing purposes)
code: examplePackageGood,
Expand Down Expand Up @@ -300,7 +310,8 @@ ruleTester.run("ts-package-json-main-is-cjs", rule, {
filename: "package.json",
errors: [
{
message: "main is set to dist//index.js when it should be set to dist/index.js",
message:
"main is set to dist//index.js when it should be set to dist/index.js or dist/index.cjs",
},
],
output: '{"main": "dist/index.js"}',
Expand All @@ -310,7 +321,8 @@ ruleTester.run("ts-package-json-main-is-cjs", rule, {
filename: "package.json",
errors: [
{
message: "main is set to .dist/index.js when it should be set to dist/index.js",
message:
"main is set to .dist/index.js when it should be set to dist/index.js or dist/index.cjs",
},
],
output: '{"main": "dist/index.js"}',
Expand All @@ -320,7 +332,8 @@ ruleTester.run("ts-package-json-main-is-cjs", rule, {
filename: "package.json",
errors: [
{
message: "main is set to /dist/index.js when it should be set to dist/index.js",
message:
"main is set to /dist/index.js when it should be set to dist/index.js or dist/index.cjs",
},
],
output: '{"main": "dist/index.js"}',
Expand All @@ -331,7 +344,7 @@ ruleTester.run("ts-package-json-main-is-cjs", rule, {
filename: "package.json",
errors: [
{
message: "main is set to dist when it should be set to dist/index.js",
message: "main is set to dist when it should be set to dist/index.js or dist/index.cjs",
},
],
output: '{"main": "dist/index.js"}',
Expand All @@ -341,7 +354,8 @@ ruleTester.run("ts-package-json-main-is-cjs", rule, {
filename: "package.json",
errors: [
{
message: "main is set to index.js when it should be set to dist/index.js",
message:
"main is set to index.js when it should be set to dist/index.js or dist/index.cjs",
},
],
output: '{"main": "dist/index.js"}',
Expand All @@ -351,7 +365,8 @@ ruleTester.run("ts-package-json-main-is-cjs", rule, {
filename: "package.json",
errors: [
{
message: "main is set to dist/src/index.js when it should be set to dist/index.js",
message:
"main is set to dist/src/index.js when it should be set to dist/index.js or dist/index.cjs",
},
],
output: '{"main": "dist/index.js"}',
Expand All @@ -362,7 +377,8 @@ ruleTester.run("ts-package-json-main-is-cjs", rule, {
filename: "package.json",
errors: [
{
message: "main is set to index.js when it should be set to dist/index.js",
message:
"main is set to index.js when it should be set to dist/index.js or dist/index.cjs",
},
],
output: examplePackageGood,
Expand Down
4 changes: 3 additions & 1 deletion sdk/core/core-rest-pipeline/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
# Release History

## 1.9.2 (Unreleased)
## 1.10.0 (Unreleased)

### Features Added

- Added conditional exports for CommonJS and ESM. [#22804](https://github.com/Azure/azure-sdk-for-js/pull/22804)

### Breaking Changes

### Bugs Fixed
Expand Down
34 changes: 24 additions & 10 deletions sdk/core/core-rest-pipeline/package.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
{
"name": "@azure/core-rest-pipeline",
"version": "1.9.2",
"version": "1.10.0",
"description": "Isomorphic client library for making HTTP requests in node.js and browser.",
"sdk-type": "client",
"main": "dist/index.js",
"main": "dist/index.cjs",
"module": "dist-esm/src/index.js",
"type": "module",
"export": {
".": {
"types": "core-rest-pipeline.shims.d.ts",
"require": "./dist/index.cjs",
"import": "./dist-esm/src/index.js"
}
},
"browser": {
"./dist-esm/src/defaultHttpClient.js": "./dist-esm/src/defaultHttpClient.browser.js",
"./dist-esm/src/policies/decompressResponsePolicy.js": "./dist-esm/src/policies/decompressResponsePolicy.browser.js",
Expand All @@ -14,7 +22,7 @@
"./dist-esm/src/util/userAgentPlatform.js": "./dist-esm/src/util/userAgentPlatform.browser.js"
},
"react-native": {
"./dist/index.js": "./dist-esm/src/index.js",
"./dist/index.cjs": "./dist-esm/src/index.js",
"./dist-esm/src/defaultHttpClient.js": "./dist-esm/src/defaultHttpClient.native.js",
"./dist-esm/src/util/userAgentPlatform.js": "./dist-esm/src/util/userAgentPlatform.native.js"
},
Expand All @@ -29,9 +37,9 @@
"scripts": {
"audit": "node ../../../common/scripts/rush-audit.js && rimraf node_modules package-lock.json && npm i --package-lock-only 2>&1 && npm audit",
"build:samples": "echo Obsolete",
"build:test": "tsc -p . && dev-tool run bundle",
"build:test": "tsc -p . && dev-tool run bundle --output-cjs-ext=true",
"build:types": "downlevel-dts types/latest/ types/3.1/",
"build": "npm run clean && tsc -p . && dev-tool run bundle && api-extractor run --local && npm run build:types",
"build": "npm run clean && tsc -p . && dev-tool run bundle --output-cjs-ext=true && api-extractor run --local && npm run build:types",
"check-format": "prettier --list-different --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"",
"clean": "rimraf dist dist-* temp types *.tgz *.log",
"execute:samples": "echo skipped",
Expand All @@ -45,15 +53,17 @@
"pack": "npm pack 2>&1",
"test:browser": "npm run clean && npm run build:test && npm run unit-test:browser && npm run integration-test:browser",
"test:node": "npm run clean && tsc -p . && npm run unit-test:node && npm run integration-test:node",
"test": "npm run clean && tsc -p . && npm run unit-test:node && dev-tool run bundle && npm run unit-test:browser && npm run integration-test",
"unit-test:browser": "karma start --single-run",
"unit-test:node": "mocha -r esm -r ts-node/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 1200000 --full-trace --exclude \"test/**/browser/*.spec.ts\" \"test/**/*.spec.ts\"",
"test": "npm run clean && tsc -p . && npm run unit-test:node && dev-tool run bundle --output-cjs-ext=true && npm run unit-test:browser && npm run integration-test",
"unit-test:browser": "karma start karma.conf.cjs --single-run",
"unit-test:node": "mocha --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 1200000 --full-trace --exclude \"test/**/browser/*.spec.ts\" \"test/**/*.spec.ts\"",
"unit-test": "npm run unit-test:node && npm run unit-test:browser"
},
"files": [
"dist/",
"dist-esm/src/",
"types/3.1/src/",
"types/3.1/core-rest-pipeline.d.ts",
"types/latest/src/",
"types/latest/core-rest-pipeline.d.ts",
"core-rest-pipeline.shims.d.ts",
"core-rest-pipeline.shims-3_1.d.ts",
Expand Down Expand Up @@ -125,15 +135,19 @@
"karma-sourcemap-loader": "^0.3.8",
"karma": "^6.3.0",
"mocha-junit-reporter": "^2.0.0",
"mocha": "^7.1.1",
"mocha": "^10.0.0",
"prettier": "^2.5.1",
"puppeteer": "^14.0.0",
"rimraf": "^3.0.0",
"sinon": "^9.0.2",
"source-map-support": "^0.5.9",
"typescript": "~4.6.0",
"ts-node": "^10.0.0",
"typescript": "~4.7.0",
"util": "^0.12.1"
},
"mocha": {
"loader": "ts-node/esm"
},
"//sampleConfiguration": {
"skipFolder": true,
"disableDocsMs": true,
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/core-rest-pipeline/src/constants.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

export const SDK_VERSION: string = "1.9.2";
export const SDK_VERSION: string = "1.10.0";

export const DEFAULT_RETRY_POLICY_COUNT = 3;
26 changes: 13 additions & 13 deletions sdk/core/core-rest-pipeline/src/createPipelineFromOptions.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { LogPolicyOptions, logPolicy } from "./policies/logPolicy";
import { Pipeline, createEmptyPipeline } from "./pipeline";
import { PipelineRetryOptions, TlsSettings } from "./interfaces";
import { RedirectPolicyOptions, redirectPolicy } from "./policies/redirectPolicy";
import { UserAgentPolicyOptions, userAgentPolicy } from "./policies/userAgentPolicy";
import { LogPolicyOptions, logPolicy } from "./policies/logPolicy.js";
import { Pipeline, createEmptyPipeline } from "./pipeline.js";
import { PipelineRetryOptions, TlsSettings } from "./interfaces.js";
import { RedirectPolicyOptions, redirectPolicy } from "./policies/redirectPolicy.js";
import { UserAgentPolicyOptions, userAgentPolicy } from "./policies/userAgentPolicy.js";

import { ProxySettings } from ".";
import { decompressResponsePolicy } from "./policies/decompressResponsePolicy";
import { defaultRetryPolicy } from "./policies/defaultRetryPolicy";
import { formDataPolicy } from "./policies/formDataPolicy";
import { ProxySettings } from "./interfaces.js";
import { decompressResponsePolicy } from "./policies/decompressResponsePolicy.js";
import { defaultRetryPolicy } from "./policies/defaultRetryPolicy.js";
import { formDataPolicy } from "./policies/formDataPolicy.js";
import { isNode } from "@azure/core-util";
import { proxyPolicy } from "./policies/proxyPolicy";
import { setClientRequestIdPolicy } from "./policies/setClientRequestIdPolicy";
import { tlsPolicy } from "./policies/tlsPolicy";
import { tracingPolicy } from "./policies/tracingPolicy";
import { proxyPolicy } from "./policies/proxyPolicy.js";
import { setClientRequestIdPolicy } from "./policies/setClientRequestIdPolicy.js";
import { tlsPolicy } from "./policies/tlsPolicy.js";
import { tracingPolicy } from "./policies/tracingPolicy.js";

/**
* Defines options that are used to configure the HTTP pipeline for
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { HttpClient } from "./interfaces";
import { createFetchHttpClient } from "./fetchHttpClient";
import { HttpClient } from "./interfaces.js";
import { createFetchHttpClient } from "./fetchHttpClient.js";

/**
* Create the correct HttpClient for the current environment.
Expand Down
4 changes: 2 additions & 2 deletions sdk/core/core-rest-pipeline/src/defaultHttpClient.native.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { HttpClient } from "./interfaces";
import { createXhrHttpClient } from "./xhrHttpClient";
import { HttpClient } from "./interfaces.js";
import { createXhrHttpClient } from "./xhrHttpClient.js";

/**
* Create the correct HttpClient for the current environment.
Expand Down
Loading