build(client-tree): exports with ESM as primary #19141

jason-ha · 2024-01-08T00:59:00Z

Configure full ESM build and declaration for tree
Set exports (only "public" and internal as there are no others)
- "@internal" uses of tree updated to import "/internal"
Build CommonJS secondarily with injected CommonJS package.json at build

A module package.json is also injected into ESM build for symmetry.

ESM api-extractor tsconfig is overwritten to use Bundler resolution as Node16 does not appear to be robustly handled.

- exports not specified - API extractor for CJS does not work properly

packages/dds/tree/package.json

msfluid-bot · 2024-01-08T01:32:32Z

Baseline CI build failed, cannot generate bundle analysis at this time

Baseline commit: 4c94050

Generated by 🚫 dangerJS against ccd598b

remove unused build:gen-version

make sure cjs rollup uses cjs config override moduleResolution for esm as api-extractor doesn't handle node16 robustly. It will generate with imports to invalid paths.

packages/dds/tree/api-extractor.json

use Bundler for moduleResolution with ESNext

common/build/build-common/tsc-multi.cjs.json

packages/dds/tree/package.json

packages/dds/tree/tsconfig.base.json

echo quote emit syntax is different per shell. Instead copy a file.

Add comments for module/moduleResolution settings and make moduleResolution explicit. Also inject package.json for ESM for symmetry.

tylerbutler · 2024-01-18T06:17:11Z

I dug up the comment that describes some drawbacks to this approach:

microsoft/TypeScript#54593 (comment)

I don't think this is an immediate blocker for us, but it makes me think this is a design that we'll still have to revisit later. Maybe just something we note in the readme or dev notes and revisit when it becomes a practical blocker.

jason-ha · 2024-01-18T18:31:24Z

I dug up the comment that describes some drawbacks to this approach:

microsoft/TypeScript#54593 (comment)

I don't think this is an immediate blocker for us, but it makes me think this is a design that we'll still have to revisit later. Maybe just something we note in the readme or dev notes and revisit when it becomes a practical blocker.

I was emitting lib/package.json for complete symmetry and we can do that or not. I think nice to have for now to help catch any issues from this structure. We don't use "imports" feature and I don't see the need to; so, we can add a policy to block. As noted in the referenced thread it is possible to enhance those injected package.json file with other information if needed.

I am hoping that we are really working toward an ESM only future and would then be able to drop secondary CJS build. If we can't before there is issue with lib/package.json then we will have option to drop it with current configuration.

Those seem more tractable than using tools that we can't rely on as much as tsc.

Last weekend I was playing around with addition to tsc-multi to hook package.json reads and override type. Using it for just that feature seems safe to me. (No extension or file rewrites.)

tylerbutler · 2024-01-29T21:35:24Z

packages/dds/tree/api-extractor-cjs.json

+	"compiler": {
+		"tsconfigFilePath": "<projectFolder>/tsconfig.cjs.json"
+	},
+	"apiReport": {
+		"enabled": false
+	}


Should these be defined in the base config? If not, add some comments to this file with some explanation.

I am not sure what the question is.

Should tsconfigFilePath be set in build-common/api-extractor-base.cjs*.json? No each package may have its own tsconfig setup at the moment.

Should apiReport be disabled in build-common/api-extractor-base.json? I think it should probably be off in there. The -base.*.primary.json files however would want it on. This config wants the extension neutral (.ts) settings from .primary.json but not the apiReport.

tylerbutler · 2024-01-29T21:36:00Z

packages/dds/tree/api-extractor-esm.json

+	"compiler": {
+		"overrideTsconfig": {
+			"$schema": "http://json.schemastore.org/tsconfig",
+			"extends": "./tsconfig.esnext.json",
+			"compilerOptions": {
+				"module": "esnext",
+				"moduleResolution": "Bundler"
+			}
+		}
+	}


Same question here.

tylerbutler · 2024-01-29T21:37:04Z

packages/dds/tree/api-extractor.json

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


Why does this extend from api-extractor-base while the others use different bases?

Is this needed? Seems like the package.json scripts explicitly pass a config file.

Everything eventually extends from api-extractor-base. The other build-common files extend api-extractor-base, set entry point, set rollup files, and if not primary disable apiReport.

build:docs and ci:build:docs use this file implicitly.

packages/dds/tree/package.json

tylerbutler · 2024-01-31T00:01:23Z

packages/dds/tree/package.json

@@ -11,18 +11,42 @@
 	"license": "MIT",
 	"author": "Microsoft and contributors",
 	"sideEffects": false,
+	"type": "module",


Isn't this a bit dangerous?

I don't know that this is dangerous. Why might it be dangerous?

(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.)

tylerbutler · 2024-01-31T00:04:45Z

packages/dds/tree/package.json

@@ -46,7 +70,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",


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.

I did enjoy fluid-build being smart about this. I am not sure the long-term desired breakup. I guess tsc would kick off the two tasks b/c we want to keep the main build command simple and common. So, we have tsc that does tsc:transpile and tsc:setcjs later?

packages/dds/tree/tsconfig.cjs.json

jason-ha · 2024-02-09T22:03:28Z

Ah timing. Looks like some non-public use of tree snuck in. exports will need adjusted (if not removed).

alexvy86

Just looked at the Devtools change in detail. Can't vouch for the rest :)

alexvy86 · 2024-02-10T00:03:04Z

packages/tools/devtools/devtools-core/src/data-visualization/DefaultVisualizers.ts

+import type { ISharedTree } from "@fluidframework/tree/internal";
+import { SharedTree, encodeTreeSchema } from "@fluidframework/tree/internal";


Just surprised to see these are all internal now. Are we sure not even ISharedTree should be public?

- Configure full ESM build and declaration for tree - Set exports (only "public" and `internal` as there are no others) - "@internal" uses of tree updated to import "/internal" - Build CommonJS secondarily with injected CommonJS package.json at build A module package.json is also injected into ESM build for symmetry. ESM api-extractor tsconfig is overwritten to use `Bundler` resolution as `Node16` does not appear to be robustly handled.

jason-ha added 6 commits January 7, 2024 15:21

build: tsc-multi for cjs renames .d.ts

e07e8a8

build(tree): settings for ESM as primary

c1e6092

- exports not specified - API extractor for CJS does not work properly

build(client-tree): CJS via tsc

7c75513

test(client-tree): test commonjs

ee1576a

build(client-tree): exclude fluid-build-tasks-tsc policy

7c064e0

build(client-tree): set package exports

c0f68c0

github-actions bot added area: build Build related issues area: dds Issues related to distributed data structures area: dds: tree base: main PRs targeted against main branch labels Jan 8, 2024

jason-ha commented Jan 8, 2024

View reviewed changes

packages/dds/tree/package.json Outdated Show resolved Hide resolved

jason-ha added 3 commits January 8, 2024 00:55

build(client-tree): cleanup build dependencies

b6d1d5e

build(client-tree): full ESM build

a9bccf8

build(client-tree): add api task

8978bdd

remove unused build:gen-version

github-actions bot added the public api change Changes to a public API label Jan 8, 2024

build(client-tree): fix api report/rollup

4d252b7

make sure cjs rollup uses cjs config override moduleResolution for esm as api-extractor doesn't handle node16 robustly. It will generate with imports to invalid paths.

github-actions bot removed the public api change Changes to a public API label Jan 8, 2024

jason-ha mentioned this pull request Jan 8, 2024

build(tree): Make compatible with moduleResolution: node16 #18882

Closed

tylerbutler reviewed Jan 8, 2024

View reviewed changes

packages/dds/tree/api-extractor.json Outdated Show resolved Hide resolved

build(client-tree): improve api report/rollup

5655ba2

use Bundler for moduleResolution with ESNext

jason-ha marked this pull request as ready for review January 8, 2024 21:19

jason-ha requested review from msfluid-bot and a team as code owners January 8, 2024 21:19

tylerbutler reviewed Jan 8, 2024

View reviewed changes

common/build/build-common/tsc-multi.cjs.json Outdated Show resolved Hide resolved

packages/dds/tree/package.json Show resolved Hide resolved

packages/dds/tree/tsconfig.base.json Show resolved Hide resolved

revert(build-common): don't update tsc-multi.cjs.json

eef3130

github-actions bot removed the area: build Build related issues label Jan 8, 2024

jason-ha requested a review from alexvy86 January 8, 2024 22:02

jason-ha requested a review from tylerbutler January 9, 2024 01:07

jason-ha added 2 commits January 10, 2024 17:24

merge: main with tree-exports-with-esm-as-primary

4ff9896

merge: main with tree-exports-with-esm-as-primary

000d231

github-actions bot added area: build Build related issues dependencies Pull requests that update a dependency file labels Jan 16, 2024

jason-ha added 2 commits January 16, 2024 16:12

fix(tree): fix CJS package.json injection

31c512b

echo quote emit syntax is different per shell. Instead copy a file.

build(client-tree): CJS explicit moduleResolution

0bcd137

Add comments for module/moduleResolution settings and make moduleResolution explicit. Also inject package.json for ESM for symmetry.

jason-ha force-pushed the tree-exports-with-esm-as-primary branch from 6870b1a to 0bcd137 Compare January 17, 2024 00:54

tylerbutler reviewed Jan 31, 2024

View reviewed changes

jason-ha added 3 commits February 2, 2024 22:55

merge: main with tree-exports-with-esm-as-primary

695cb0c

build(client-tree): restore build:genver

40bdc4b

merge: main with tree-exports-with-esm-as-primary

c8b2779

jason-ha requested a review from a team as a code owner February 7, 2024 19:37

tylerbutler approved these changes Feb 9, 2024

View reviewed changes

Merge branch 'main' into tree-exports-with-esm-as-primary

7c5a038

jason-ha added 2 commits February 9, 2024 14:36

Merge branch 'main' into tree-exports-with-esm-as-primary

3bc2c4c

build(client): fix internal tree imports

ccd598b

jason-ha requested a review from a team as a code owner February 9, 2024 22:54

github-actions bot added the area: dds: propertydds label Feb 9, 2024

alexvy86 approved these changes Feb 10, 2024

View reviewed changes

jason-ha merged commit fddd68b into microsoft:main Feb 10, 2024
47 checks passed

jason-ha deleted the tree-exports-with-esm-as-primary branch February 21, 2024 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(client-tree): exports with ESM as primary #19141

build(client-tree): exports with ESM as primary #19141

jason-ha commented Jan 8, 2024 •

edited

Loading

msfluid-bot commented Jan 8, 2024 •

edited

Loading

tylerbutler commented Jan 18, 2024

jason-ha commented Jan 18, 2024

tylerbutler Jan 29, 2024

jason-ha Feb 3, 2024

tylerbutler Jan 29, 2024

tylerbutler Jan 29, 2024

tylerbutler Jan 31, 2024

jason-ha Feb 3, 2024 •

edited

Loading

tylerbutler Jan 31, 2024

jason-ha Jan 31, 2024

jason-ha Jan 31, 2024

tylerbutler Jan 31, 2024

jason-ha Feb 7, 2024

jason-ha commented Feb 9, 2024

alexvy86 left a comment

alexvy86 Feb 10, 2024

		import type { ISharedTree } from "@fluidframework/tree/internal";
		import { SharedTree, encodeTreeSchema } from "@fluidframework/tree/internal";

build(client-tree): exports with ESM as primary #19141

build(client-tree): exports with ESM as primary #19141

Conversation

jason-ha commented Jan 8, 2024 • edited Loading

msfluid-bot commented Jan 8, 2024 • edited Loading

tylerbutler commented Jan 18, 2024

jason-ha commented Jan 18, 2024

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

jason-ha Feb 3, 2024 • edited Loading

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

jason-ha commented Feb 9, 2024

alexvy86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jason-ha commented Jan 8, 2024 •

edited

Loading

msfluid-bot commented Jan 8, 2024 •

edited

Loading

jason-ha Feb 3, 2024 •

edited

Loading