-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from 6 commits
ee27d04
1c0c4fb
db7e7ef
aa393d5
31632f4
db5ed1f
d9fe9ee
d874bc1
65524e6
76a44b5
5231081
279160c
00d9c25
6bcb49c
10f3fa6
5bb5b13
ccc1365
57705cf
df32ab1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,8 +3,16 @@ | |||||
"version": "1.9.1", | ||||||
"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": "./types/src/index.js", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't this still be something like
Suggested change
Though really shouldn't this match the main types entry so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copied from Matt's PR. Will fix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mpodwysocki the "types" export in notification hub package.json seem incorrect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They resolve just fine in TypeScript as is. I got the advice from @DanielRosenwasser and it works. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"
}
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@weswigham thanks for the detail information! For our packages we used a rollup d.ts for the default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, from the linked discussion microsoft/TypeScript#50466 we would need two separate files so that the correct mode is applied. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated this PR to have two copies of the same shim type file, |
||||||
"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", | ||||||
|
@@ -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" | ||||||
}, | ||||||
|
@@ -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", | ||||||
|
@@ -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", | ||||||
|
@@ -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, | ||||||
|
There was a problem hiding this comment.
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?
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.There was a problem hiding this comment.
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.