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

jeremymeng · 2022-08-04T18:20:18Z

Add conditional exports for core-rest-pipeline
Add .js extension to imported source module name
Bump dev dependency Mocha version to v10
Add ts-node/esm loader in mocha config
Update npm scripts
Update linter rule to allow dist/index.cjs
Add a copy of our typing files for cjs
Bump minor version

Packages impacted by this PR

@azure/core-rest-pipeline

Issues associated with this PR

#22172

- when bundle, output to index.cjs - add conditional export for "require" and "import"

- rename karma.conf.js to karma.conf.cjs - update npm scripts

check-enforcer · 2022-08-04T18:30:30Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

azure-sdk · 2022-08-04T18:49:32Z

API change check

API changes are not detected in this pull request.

xirzec

Very exciting stuff! I am eager to get this landed.

A couple minor comments/questions.

xirzec · 2022-08-04T19:25:50Z

sdk/core/core-rest-pipeline/test/node/nodeHttpClient.spec.ts

@@ -71,7 +71,7 @@ describe("NodeHttpClient", function () {
    const client = createDefaultHttpClient();
    const clientRequest = createRequest();
    stubbedHttpsRequest.returns(clientRequest);
-    const request = createPipelineRequest({ url: "https://example.com" });
+    const request = createPipelineRequest({ url: "https://whateverhahaha.com" });


lol is example.com banned or something?

haha, will revert. I was debugging some issue and the request went to the wire and gave me 200, so I changed temporarily but forgot to restore.

xirzec · 2022-08-04T19:26:06Z

sdk/core/core-rest-pipeline/test/node/pipeline.spec.ts

@@ -1,19 +1,20 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.

-import * as http from "https";
-import * as https from "https";
+import http from "https";


is the * as foo syntax just not required anymore?

I am not sure about the root cause yet, but sinon cannot mock import * as and gave this error: "ES Modules cannot be stubbed". I had to change the import in the src files as well, otherwise, sinon is not able to modify http.request.

xirzec · 2022-08-04T19:28:57Z

sdk/core/core-rest-pipeline/package.json

+  "type": "module",
+  "export": {
+    ".": {
+      "types": "./types/src/index.js",


wouldn't this still be something like

Suggested change

"types": "./types/src/index.js",

"types": "./types/latest/src/index.d.ts",

Though really shouldn't this match the main types entry so core-rest-pipeline.shims.d.ts ?

Copied from Matt's PR. Will fix.

@mpodwysocki the "types" export in notification hub package.json seem incorrect?

They resolve just fine in TypeScript as is. I got the advice from @DanielRosenwasser and it works.

@mpodwysocki does TS not need this to resolve to a d.ts then? I guess I'm confused how the resolution works, lmk if you can point me at some docs

You can actually nest conditions arbitrarily deep, and use arrays of fallbacks, rather than just simple strings! They're quite complicated! Anyways, it would look something like

"exports": { ".": { "import": { "types": "./types/index.d.ts", "default": "./dist/index.js" }, "require": { "types": "./types/index.d.cts", "default": "./dist/index.cjs" } } }

TIL! 😮

Thanks for taking the time to explain, hopefully once we find the right config we can tool it such that humans don't have to think too hard about this stuff. 👍

There should be two .d.ts files. One for the esm module, one for the cjs module. The esm module can (and probably should) just re-export stuff from the cjs one if it helps.

@weswigham thanks for the detail information! For our packages we used a rollup d.ts for the default types field. From my testing it worked when both require and import use the same d.ts file. Is it necessary to have two separate files?

okay, from the linked discussion microsoft/TypeScript#50466 we would need two separate files so that the correct mode is applied.

I updated this PR to have two copies of the same shim type file, d.ts for import, and d.cts for require. I tested the package on two projects, an ESM one with "type": "module", and a CJS one without "type": "module". Result of VS Code IntelliSense/Go to Definition work the same as before so I assume this should be good.

witemple-msft

I have a tiny change to request for dev-tool but I think it's important.

witemple-msft · 2022-10-11T20:16:04Z

common/tools/dev-tool/src/commands/run/bundle.ts

@@ -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"}`,


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.

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.

and use the `main` entry value in package.json instead.

witemple-msft

Nice! LGTM.

xirzec

Thanks for tackling this!

- Add conditional exports for core-rest-pipeline - Add .js extension to imported source module name - Bump dev dependency Mocha version to v10 - Add `ts-node/esm` loader in mocha config - Update npm scripts - Update linter rule to allow dist/index.cjs - Add a copy of our typing files for cjs - Bump minor version ### Packages impacted by this PR `@azure/core-rest-pipeline` ### Issues associated with this PR #22172

This reverts commit 02f6ad6.

This reverts commit 02f6ad6. ### Packages impacted by this PR `@azure/core-rest-pipeline` ### Describe the problem that is addressed by this PR Making core-rest-pipeline ESM with conditional exports broke notification-hubs. Reverting the change while investigating.

jeremymeng added 5 commits August 1, 2022 20:28

Conditional export for core-rest-pipeline

ee27d04

- when bundle, output to index.cjs - add conditional export for "require" and "import"

Add an option to dev-tool bundle command for .cjs output extension

1c0c4fb

import "*" => import "*.js"

db7e7ef

bump mocha version to ^10.0.0

aa393d5

- import * as => import

31632f4

- rename karma.conf.js to karma.conf.cjs - update npm scripts

jeremymeng requested review from praveenkuttappan, witemple-msft, deyaaeldeen, mikeharder and xirzec as code owners August 4, 2022 18:20

ghost added Azure.Core dev-tool Issues related to the Azure SDK for JS dev-tool labels Aug 4, 2022

Merge remote-tracking branch 'up/main' into core/cond-export

db5ed1f

jeremymeng requested review from joheredi and mpodwysocki August 4, 2022 18:43

xirzec changed the title ~~[core-rest-pipeline]~~ [core-rest-pipeline] Add subpath exports Aug 4, 2022

xirzec reviewed Aug 4, 2022

View reviewed changes

jeremymeng added 3 commits August 4, 2022 20:01

- Fix linter rule to allow "main": "dist/index.cjs"

d9fe9ee

revert back to awesome example.com

d874bc1

Fix types export

65524e6

jeremymeng changed the title ~~[core-rest-pipeline] Add subpath exports~~ [core-rest-pipeline] Add conditional exports Aug 4, 2022

jeremymeng added 3 commits August 4, 2022 23:10

Merge remote-tracking branch 'up/main' into core/cond-export

76a44b5

Bump minor version and update CHANGELOG

5231081

Merge remote-tracking branch 'up/main' into core/cond-export

279160c

jeremymeng mentioned this pull request Aug 5, 2022

Can't import the named export 'RestError' from non EcmaScript module (only default export is available) #22172

Open

DanielRosenwasser mentioned this pull request Aug 26, 2022

NodeNext resolution failed to resolve dual-package correctly microsoft/TypeScript#50466

Open

jeremymeng added 2 commits August 29, 2022 19:12

Merge remote-tracking branch 'up/main' into core/cond-export

00d9c25

Merge remote-tracking branch 'up/main' into core/cond-export

6bcb49c

Add copy of our types shim for CommonJS conditional export

10f3fa6

witemple-msft requested changes Oct 11, 2022

View reviewed changes

jeremymeng added 2 commits October 17, 2022 18:03

remove output-cjs-ext option

5bb5b13

and use the `main` entry value in package.json instead.

Merge remote-tracking branch 'upstream/main' into core/cond-export

ccc1365

jeremymeng requested a review from witemple-msft October 18, 2022 19:14

witemple-msft approved these changes Oct 19, 2022

View reviewed changes

xirzec approved these changes Oct 19, 2022

View reviewed changes

jeremymeng added 2 commits October 20, 2022 17:00

delete usage of removed dev-tool bundle option

57705cf

Merge remote-tracking branch 'up/main' into core/cond-export

df32ab1

jeremymeng merged commit 02f6ad6 into Azure:main Oct 20, 2022

jeremymeng deleted the core/cond-export branch October 20, 2022 17:45

jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this pull request Nov 3, 2022

Revert "[core-rest-pipeline] Add conditional exports (Azure#22804)"

fad60a4

This reverts commit 02f6ad6.

unional mentioned this pull request Jan 3, 2023

This expression is not callable for ESM consuming CJS with default export microsoft/TypeScript#52086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jeremymeng commented Aug 4, 2022 •

edited

check-enforcer bot commented Aug 4, 2022

azure-sdk commented Aug 4, 2022

xirzec left a comment

xirzec Aug 4, 2022

jeremymeng Aug 4, 2022

xirzec Aug 4, 2022

jeremymeng Aug 4, 2022

xirzec Aug 4, 2022

jeremymeng Aug 4, 2022

jeremymeng Aug 4, 2022

mpodwysocki Aug 4, 2022

xirzec Aug 4, 2022

weswigham Aug 6, 2022

xirzec Aug 8, 2022

jeremymeng Aug 29, 2022

jeremymeng Aug 29, 2022

jeremymeng Oct 5, 2022

witemple-msft left a comment

witemple-msft Oct 11, 2022

jeremymeng Oct 17, 2022

witemple-msft left a comment

xirzec left a comment

	"types": "./types/src/index.js",
	"types": "./types/latest/src/index.d.ts",

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

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

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

Conversation

jeremymeng commented Aug 4, 2022 • edited

Packages impacted by this PR

Issues associated with this PR

check-enforcer bot commented Aug 4, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

azure-sdk commented Aug 4, 2022

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witemple-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witemple-msft left a comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

jeremymeng commented Aug 4, 2022 •

edited