-
Notifications
You must be signed in to change notification settings - Fork 536
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
build(client-tree): exports with ESM as primary #19141
Changes from all commits
e07e8a8
c1e6092
7c75513
ee1576a
7c064e0
c0f68c0
b6d1d5e
a9bccf8
8978bdd
4d252b7
5655ba2
eef3130
4ff9896
000d231
31c512b
0bcd137
695cb0c
40bdc4b
c8b2779
7c5a038
3bc2c4c
ccd598b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"type": "commonjs" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"type": "module" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
"$schema": "https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json", | ||
"extends": "../../../common/build/build-common/api-extractor-base.cjs.primary.json", | ||
"compiler": { | ||
"tsconfigFilePath": "<projectFolder>/tsconfig.cjs.json" | ||
}, | ||
"apiReport": { | ||
"enabled": false | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
{ | ||
"$schema": "https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json", | ||
"extends": "../../../common/build/build-common/api-extractor-base.esm.primary.json", | ||
"compiler": { | ||
"overrideTsconfig": { | ||
"$schema": "http://json.schemastore.org/tsconfig", | ||
"extends": "./tsconfig.esnext.json", | ||
"compilerOptions": { | ||
"module": "esnext", | ||
"moduleResolution": "Bundler" | ||
} | ||
} | ||
} | ||
Comment on lines
+4
to
+13
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. Same question here. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
{ | ||
"$schema": "https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json", | ||
"extends": "../../../common/build/build-common/api-extractor-lint.json" | ||
"extends": "../../../common/build/build-common/api-extractor-lint.esm.primary.json" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,15 @@ | ||
{ | ||
"$schema": "https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json", | ||
"extends": "../../../common/build/build-common/api-extractor-base.json" | ||
"extends": "../../../common/build/build-common/api-extractor-base.json", | ||
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. Why does this extend from api-extractor-base while the others use different bases? 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. Is this needed? Seems like the package.json scripts explicitly pass a config file. 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.
|
||
"mainEntryPointFilePath": "./lib/index.d.ts", | ||
"compiler": { | ||
"overrideTsconfig": { | ||
"$schema": "http://json.schemastore.org/tsconfig", | ||
"extends": "./tsconfig.esnext.json", | ||
"compilerOptions": { | ||
"module": "esnext", | ||
"moduleResolution": "Bundler" | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,17 +11,42 @@ | |
"license": "MIT", | ||
"author": "Microsoft and contributors", | ||
"sideEffects": false, | ||
"type": "module", | ||
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. Isn't this a bit dangerous? 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 don't know that this is dangerous. Why might it be dangerous? 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 haven't requested the tsc-multi feature that I think it needs. Without tsc seeing "type": "module" it doesn't go into full ESM mode. This is a must for now to ensure proper ESM transpilation.) |
||
"exports": { | ||
".": { | ||
"import": { | ||
"types": "./lib/tree-public.d.ts", | ||
jason-ha marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"default": "./lib/index.js" | ||
}, | ||
"require": { | ||
"types": "./dist/tree-public.d.ts", | ||
"default": "./dist/index.js" | ||
} | ||
}, | ||
"./internal": { | ||
"import": { | ||
"types": "./lib/index.d.ts", | ||
"default": "./lib/index.js" | ||
}, | ||
"require": { | ||
"types": "./dist/index.d.ts", | ||
"default": "./dist/index.js" | ||
} | ||
} | ||
}, | ||
"main": "dist/index.js", | ||
"module": "lib/index.js", | ||
"types": "dist/index.d.ts", | ||
"scripts": { | ||
"api": "fluid-build . --task api", | ||
"api-extractor:commonjs": "api-extractor run --config ./api-extractor-cjs.json", | ||
"api-extractor:esnext": "api-extractor run --local --config ./api-extractor-esm.json", | ||
"bench": "mocha --timeout 999999 --perfMode --parentProcess --fgrep @Benchmark --reporter @fluid-tools/benchmark/dist/MochaReporter.js", | ||
"bench:profile": "mocha --v8-prof --v8-logfile=profile.log --v8-no-logfile-per-isolate --timeout 999999 --perfMode --fgrep @Benchmark --reporter @fluid-tools/benchmark/dist/MochaReporter.js && node --prof-process profile.log > profile.txt && rimraf profile.log && echo See results in profile.txt", | ||
"build": "fluid-build . --task build", | ||
"build:commonjs": "fluid-build . --task commonjs", | ||
"build:compile": "fluid-build . --task compile", | ||
"build:docs": "api-extractor run --local", | ||
"build:esnext": "tsc --project ./tsconfig.esnext.json", | ||
"build:esnext": "tsc --project ./tsconfig.esnext.json && copyfiles -f ../../../common/build/build-common/src/esm/package.json ./lib", | ||
"build:genver": "gen-version", | ||
jason-ha marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"build:test": "tsc --project ./src/test/tsconfig.json", | ||
"check:are-the-types-wrong": "attw --pack . --entrypoints .", | ||
|
@@ -46,7 +71,7 @@ | |
"test:snapshots:regen": "npm run test:mocha -- --snapshot", | ||
"test:stress": "cross-env FUZZ_TEST_COUNT=20 FUZZ_STRESS_RUN=true mocha --ignore \"dist/test/memory/**/*\" --recursive \"dist/test/**/*.spec.js\" -r @fluidframework/mocha-test-setup", | ||
"ts2esm": "ts2esm ./tsconfig.json ./src/test/tsconfig.json", | ||
"tsc": "tsc", | ||
"tsc": "tsc --project ./tsconfig.cjs.json && copyfiles -f ../../../common/build/build-common/src/cjs/package.json ./dist", | ||
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 think this is fine for now since fluid-build understands tsc and copyfiles and should decompose this into two dependent tasks (one for tsc and the other for copyfiles). However, longer-term I think the pattern we should use is to have explicit separate tasks for the copying and make those dependencies explicit in the fluid-build config as well. 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 did enjoy fluid-build being smart about this. I am not sure the long-term desired breakup. I guess |
||
"typetests:gen": "fluid-type-test-generator", | ||
"typetests:prepare": "flub typetests --dir . --reset --previous --normalize" | ||
}, | ||
|
@@ -112,6 +137,7 @@ | |
"ajv": "^8.12.0", | ||
"ajv-formats": "^2.1.1", | ||
"c8": "^8.0.1", | ||
"copyfiles": "^2.4.1", | ||
"cross-env": "^7.0.3", | ||
"dependency-cruiser": "^14.1.0", | ||
"diff": "^3.5.0", | ||
|
@@ -127,6 +153,22 @@ | |
"ts2esm": "^1.1.0", | ||
"typescript": "~5.1.6" | ||
}, | ||
"fluidBuild": { | ||
"tasks": { | ||
"build:docs": { | ||
"dependsOn": [ | ||
"build:esnext" | ||
], | ||
"script": false | ||
}, | ||
"check:release-tags": [ | ||
"build:esnext" | ||
], | ||
"ci:build:docs": [ | ||
"build:esnext" | ||
] | ||
} | ||
}, | ||
"typeValidation": { | ||
"disabled": true, | ||
"broken": {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"extends": "@fluidframework/build-common/ts-common-config.json", | ||
jason-ha marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"exclude": ["src/test/**/*"], | ||
"compilerOptions": { | ||
"rootDir": "./src", | ||
"outDir": "./error-override-outdir-in-main-tsconfig", | ||
"noImplicitAny": true, | ||
"composite": true, | ||
"preserveConstEnums": true, | ||
// Allow unused locals. | ||
// This is needed for type assertions using the TypeCheck library. | ||
// Linter is used to enforce "_" prefix for unused locals to prevent accidentally having unused locals. | ||
"noUnusedLocals": false, | ||
"noImplicitOverride": true, | ||
}, | ||
"include": ["src/**/*"], | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"extends": "./tsconfig.base.json", | ||
"compilerOptions": { | ||
"outDir": "./dist", | ||
// Ideally there would be no CommonJS transpilation and wrappers would exist around | ||
// ESM modules as needed. Until there is such support also transpile to CommonJS. | ||
// Next preference is to use Node16 for module and moduleResolution with package.json | ||
// "type": "commonjs". However "type": "module" is required to activate full ESM Mode | ||
// for the ESM build. | ||
// If solution like tsc-multi adds support for package.json type override then this | ||
// can be Node16 with a "type": "commonjs" package.json override. | ||
// Underlying reason for the complexity here is that base tools wish to avoid the dual | ||
// package hazard of ESM and CommonJS in the same package. "tsc-multi" is a workaround. | ||
"module": "CommonJS", | ||
"moduleResolution": "Node10", | ||
jason-ha marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,10 @@ | ||
{ | ||
"extends": "./tsconfig.json", | ||
"extends": "./tsconfig.base.json", | ||
"compilerOptions": { | ||
"outDir": "./lib", | ||
"module": "esnext", | ||
// Node16 and ESNext is effectively the same for transpile so long as package.json has | ||
// "type": "module", which is required to activate full ESM Mode within tsc. | ||
"module": "Node16", | ||
"moduleResolution": "Node16", | ||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,3 @@ | ||
{ | ||
"extends": "@fluidframework/build-common/ts-common-config.json", | ||
"exclude": ["src/test/**/*"], | ||
"compilerOptions": { | ||
"rootDir": "./src", | ||
"outDir": "./dist", | ||
"noImplicitAny": true, | ||
"composite": true, | ||
"preserveConstEnums": true, | ||
// Allow unused locals. | ||
// This is needed for type assertions using the TypeCheck library. | ||
// Linter is used to enforce "_" prefix for unused locals to prevent accidentally having unused locals. | ||
"noUnusedLocals": false, | ||
"noImplicitOverride": true, | ||
}, | ||
"include": ["src/**/*"], | ||
"extends": "./tsconfig.esnext.json", | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,8 @@ import { SharedCounter } from "@fluidframework/counter"; | |
import { type IDirectory, SharedDirectory, SharedMap } from "@fluidframework/map"; | ||
import { SharedMatrix } from "@fluidframework/matrix"; | ||
import { SharedString } from "@fluidframework/sequence"; | ||
import { SharedTree, type ISharedTree, encodeTreeSchema } from "@fluidframework/tree"; | ||
import type { ISharedTree } from "@fluidframework/tree/internal"; | ||
import { SharedTree, encodeTreeSchema } from "@fluidframework/tree/internal"; | ||
Comment on lines
+16
to
+17
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. Just surprised to see these are all internal now. Are we sure not even ISharedTree should be public? |
||
import { type ISharedObject } from "@fluidframework/shared-object-base"; | ||
import { EditType } from "../CommonInterfaces"; | ||
import { type VisualizeChildData, type VisualizeSharedObject } from "./DataVisualization"; | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Should these be defined in the base config? If not, add some comments to this file with some explanation.
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.
I am not sure what the question is.