[core-http] Rework use of "lib": ["dom"] in core-http #7500
Conversation
Signed-off-by: Will Temple <witemple@microsoft.com>
willmtemple
requested review from
bterlson,
chradek,
daviwil,
jeremymeng,
ramya-rao-a and
xirzec
as code owners
| @@ -81,7 +82,7 @@ | |||
| "test": "npm run build:test && npm run unit-test && npm run integration-test", | |||
| "unit-test": "npm run unit-test:node && npm run unit-test:browser", | |||
| "unit-test:browser": "node ./node_modules/karma/bin/karma start karma.conf.ts --browsers ChromeNoSecurity --single-run", | |||
| "unit-test:node": "nyc mocha", | |||
| "unit-test:node": "cross-env TS_NODE_FILES=true nyc mocha", | |||
There was a problem hiding this comment.
Interesting, what does TS_NODE_FILES do?
There was a problem hiding this comment.
@daviwil I had no idea until a few minutes ago. It affects the way ts-node reads tsconfig.json. It causes it to load the includes, excludes, and files (it apparently does not do so by default). I'm not 100% sure why it has this effect, but in this case it is required for ts-node to read the dom.d.ts file. Previously the unit-tests were getting DOM types from lib:dom, since compilerOptions is loaded by default.
There was a problem hiding this comment.
I googled the same problem and came to the same conclusion some months ago, fwiw
| import { HttpOperationResponse } from "./httpOperationResponse"; | ||
| import { WebResource } from "./webResource"; | ||
| import { createProxyAgent, ProxyAgent, isUrlHttps } from "./proxyAgent"; | ||
|
|
||
| interface GlobalWithFetch extends NodeJS.Global { | ||
| fetch: (input: RequestInfo, init?: RequestInit) => Promise<Response>; | ||
| fetch: typeof import("node-fetch")["default"]; |
There was a problem hiding this comment.
huh, this is interesting, I don't think I've seen this style of typing off of imports before
Porting changes from @azure/core-http PR Azure/azure-sdk-for-js#7500. - Keep lib: ["dom"] for ./scripts/ as it depends on a dev common tool which depends on v10 @azure/storage-blob which depends on an older version of ms-rest-js. Attempting to remove it resulting in compile errors. This folder is only used in dev/CI. It's not part of the shipping library.
This PR is an attempt to fix some long-standing weirdness in the repo related to the use of
"lib": ["dom", ..].Related issues:
How it works
core-httpneeds a full set of DOM definitions, otherwise we would have to shim everything we use in types like XmlHttpRequest and others. Including/// <reference lib="dom" />in the newlib/dom.d.tsfile causes the full set of DOM types to be ambiently declared for the compilation ofcore-http, but this file will not be emitted to the output directory during compilation. If we had simply added the reference tocoreHttp.ts, then any consumer would also receive these definitions. By using thelib/dom.d.tsfile and referencing the full DOM types there, we avoid re-exporting all of DOM to our customers.In addition to this full declaration file, a new shim file
dom-shim.d.tsin the package root provides shims for the commonly-used interfaces (Request, RequestInit, Response, and Headers --- cc @daviwil , is there anything else to add to these default shims?).lib/coreHttp.tsreferences these shims directly using/// <reference path="../dom-shim.d.ts" />. Typescript rewrites this reference path so that it is still valid from the emittedcoreHttp.d.tsdeclaration file, and so all core-http consumers will receive this set of shims for free and will not have to re-shim.However, only
interfaces can be shimmed, so this creates a constraint on core-http, which is that we cannot export anything using a type that cannot be shimmed (see:RequestInfo). Fortunately, it isn't too difficult to just remove that type from our signatures.What all of this means is that if a package does not, itself, need DOM types internally, it won't be forced to shim or import DOM just because it uses core-http. Shimming/using the full DOM types is now only required if the package actually needs to have browser-specific behavior.
Other packages that need to use DOM can follow this same pattern of having ambient declarations of the full DOM types while referencing only required shims in the emitted declarations.
This is not a breaking change. Dependents that previously opted for either the shims-based or lib:dom-based workarounds will continue to function as they did previously with no change required.
Changes:
RequestInfofrom fetch-based http clients (RequestInfo is atype-- not aninterface, so it cannot be shimmed) and replaced them with a typeCommonRequestInfothat captures the common subset of inputs that are valid for both node-fetch and browser-based global fetchtemplateandai-text-analytics(keyvault and storage actually have browser-specific code, so they should be handled separately --- appconfig has nolibentry cc: @richardpark-msft )