-
Notifications
You must be signed in to change notification settings - Fork 532
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(tree): Make compatible with moduleResolution: node16 #18882
Changes from all commits
c2657e7
3945131
7fd1e68
9c45c01
321350b
629a5fb
5a73c61
8ae0f78
b80f612
d3fa7f6
75f7a95
165a07d
ccaa26a
6a16a5d
538353c
a1b91aa
f189eb7
058bdfb
960d61b
acf44d8
3bdad72
e10a943
325bf6f
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 |
---|---|---|
@@ -1,10 +1,11 @@ | ||
{ | ||
"extends": "@fluidframework/build-common/ts-common-config.json", | ||
"extends": [ | ||
"../../../common/build/build-common/tsconfig.base.json", | ||
"../../../common/build/build-common/tsconfig.esm-only.json", | ||
], | ||
"include": ["src/**/*", "tests/**/*"], | ||
"compilerOptions": { | ||
"module": "esnext", | ||
"outDir": "lib", | ||
"jsx": "react", | ||
"types": ["jest", "puppeteer", "jest-environment-puppeteer", "expect-puppeteer", "react"], | ||
}, | ||
"include": ["src/**/*", "tests/**/*"], | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,15 @@ | ||
{ | ||
"extends": "@fluidframework/build-common/ts-common-config.json", | ||
"extends": [ | ||
"../../../common/build/build-common/tsconfig.base.json", | ||
"../../../common/build/build-common/tsconfig.cjs.json" | ||
], | ||
"include": ["src/**/*"], | ||
"exclude": ["dist", "node_modules"], | ||
"compilerOptions": { | ||
"composite": true, | ||
"rootDir": "./src", | ||
"outDir": "./dist", | ||
"downlevelIteration": true, | ||
"noUnusedLocals": false, | ||
"outDir": "./dist", | ||
"rootDir": "./src", | ||
"types": ["mocha"] | ||
}, | ||
"include": ["src/**/*"] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"$schema": "https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json", | ||
"extends": "../../../common/build/build-common/api-extractor-base-esm.json" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ | |
|
||
```ts | ||
|
||
import { FieldKind as FieldKind_2 } from './schemaTypes.js'; | ||
import { FieldSchema as FieldSchema_2 } from './schemaTypes.js'; | ||
import { FluidObject } from '@fluidframework/core-interfaces'; | ||
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. These are not valid which means the API extractor generated .d.ts files are probably 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. Here is an example log when I had such gunk: https://dev.azure.com/fluidframework/public/_build/results?buildId=228628&view=logs&j=3dc8fd7e-4368-5a92-293e-d53cefc8c4b3&t=bfe27d5f-9219-57c0-0d67-1b2946a32921 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. It's worrisome to me that in the example you linked, the tools involved were only tsc and api-extractor. Does that imply that api-extractor itself doesn't like the Tree types? Maybe we should try updating api-extractor? Looks like the recent minor release added support for TypeScript 5.3 - https://github.com/microsoft/rushstack/blob/main/apps/api-extractor/CHANGELOG.md#7390 @Josmithr FYI. 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 was guessing it isn't a Tree type thing but something with moduleResolution support. I did a test with updated api-extractor for my branch and results were not great.
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. These errors might be related to the manual api-extractor patch that @Josmithr has been managing. Presumably that patch will need to be regenerated. 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. Good call out. The patch is listed as for a specific version but might not be. I then also found that we were forcing 5.1.6 into api-extractor per pnpm override. When I removed those two things the main runs worked. The lint case still complains about malformed bundledPackages. 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. Ha, ha. The patch is specifically about making |
||
import { IChannel } from '@fluidframework/datastore-definitions'; | ||
import { IChannelAttributes } from '@fluidframework/datastore-definitions'; | ||
|
@@ -14,13 +16,21 @@ import { IFluidDataStoreRuntime } from '@fluidframework/datastore-definitions'; | |
import { IFluidHandle } from '@fluidframework/core-interfaces'; | ||
import { IFluidLoadable } from '@fluidframework/core-interfaces'; | ||
import { IGarbageCollectionData } from '@fluidframework/runtime-definitions'; | ||
import { InsertableObjectFromSchemaRecord as InsertableObjectFromSchemaRecord_2 } from './schemaTypes.js'; | ||
import { ISharedObject } from '@fluidframework/shared-object-base'; | ||
import { ISummaryTreeWithStats } from '@fluidframework/runtime-definitions'; | ||
import { ITelemetryContext } from '@fluidframework/runtime-definitions'; | ||
import { LeafNodeSchema as LeafNodeSchema_2 } from '../feature-libraries/index.js'; | ||
import { ObjectFromSchemaRecord as ObjectFromSchemaRecord_2 } from './schemaTypes.js'; | ||
import { SchemaLibrary as SchemaLibrary_2 } from '../feature-libraries/schemaBuilderBase.js'; | ||
import { SessionSpaceCompressedId } from '@fluidframework/id-compressor'; | ||
import { StableId } from '@fluidframework/id-compressor'; | ||
import type { Static } from '@sinclair/typebox'; | ||
import { TreeNode as TreeNode_2 } from './types.js'; | ||
import { TreeNodeSchema as TreeNodeSchema_2 } from './schemaTypes.js'; | ||
import { TreeNodeSchemaClass as TreeNodeSchemaClass_2 } from './schemaTypes.js'; | ||
import type { TSchema } from '@sinclair/typebox'; | ||
import { WithType as WithType_2 } from './schemaTypes.js'; | ||
|
||
// @internal | ||
export function adaptEnum<TScope extends string, const TEnum extends Record<string, string>>(factory: SchemaFactory<TScope>, members: TEnum): (<TValue extends TEnum[keyof TEnum]>(value: TValue) => object & TreeNode & ObjectFromSchemaRecord<EmptyObject> & { | ||
|
@@ -1091,14 +1101,14 @@ export type LazyTreeNodeSchema = FlexTreeNodeSchema | (() => FlexTreeNodeSchema) | |
|
||
// @internal | ||
export const leaf: { | ||
number: LeafNodeSchema<"com.fluidframework.leaf.number", ValueSchema.Number>; | ||
boolean: LeafNodeSchema<"com.fluidframework.leaf.boolean", ValueSchema.Boolean>; | ||
string: LeafNodeSchema<"com.fluidframework.leaf.string", ValueSchema.String>; | ||
handle: LeafNodeSchema<"com.fluidframework.leaf.handle", ValueSchema.FluidHandle>; | ||
null: LeafNodeSchema<"com.fluidframework.leaf.null", ValueSchema.Null>; | ||
primitives: readonly [LeafNodeSchema<"com.fluidframework.leaf.number", ValueSchema.Number>, LeafNodeSchema<"com.fluidframework.leaf.boolean", ValueSchema.Boolean>, LeafNodeSchema<"com.fluidframework.leaf.string", ValueSchema.String>]; | ||
all: readonly [LeafNodeSchema<"com.fluidframework.leaf.handle", ValueSchema.FluidHandle>, LeafNodeSchema<"com.fluidframework.leaf.null", ValueSchema.Null>, LeafNodeSchema<"com.fluidframework.leaf.number", ValueSchema.Number>, LeafNodeSchema<"com.fluidframework.leaf.boolean", ValueSchema.Boolean>, LeafNodeSchema<"com.fluidframework.leaf.string", ValueSchema.String>]; | ||
library: SchemaLibrary; | ||
number: LeafNodeSchema_2<"com.fluidframework.leaf.number", ValueSchema.Number>; | ||
boolean: LeafNodeSchema_2<"com.fluidframework.leaf.boolean", ValueSchema.Boolean>; | ||
string: LeafNodeSchema_2<"com.fluidframework.leaf.string", ValueSchema.String>; | ||
handle: LeafNodeSchema_2<"com.fluidframework.leaf.handle", ValueSchema.FluidHandle>; | ||
null: LeafNodeSchema_2<"com.fluidframework.leaf.null", ValueSchema.Null>; | ||
primitives: readonly [LeafNodeSchema_2<"com.fluidframework.leaf.number", ValueSchema.Number>, LeafNodeSchema_2<"com.fluidframework.leaf.boolean", ValueSchema.Boolean>, LeafNodeSchema_2<"com.fluidframework.leaf.string", ValueSchema.String>]; | ||
all: readonly [LeafNodeSchema_2<"com.fluidframework.leaf.handle", ValueSchema.FluidHandle>, LeafNodeSchema_2<"com.fluidframework.leaf.null", ValueSchema.Null>, LeafNodeSchema_2<"com.fluidframework.leaf.number", ValueSchema.Number>, LeafNodeSchema_2<"com.fluidframework.leaf.boolean", ValueSchema.Boolean>, LeafNodeSchema_2<"com.fluidframework.leaf.string", ValueSchema.String>]; | ||
library: SchemaLibrary_2; | ||
}; | ||
|
||
// @internal (undocumented) | ||
|
@@ -1603,12 +1613,12 @@ export class test_RecursiveObject extends test_RecursiveObject_base { | |
} | ||
|
||
// @internal | ||
export const test_RecursiveObject_base: TreeNodeSchemaClass<"Test Recursive Domain.testObject", NodeKind.Object, object & TreeNode & ObjectFromSchemaRecord< { | ||
readonly recursive: FieldSchema<import("./schemaTypes.js").FieldKind.Optional, readonly [() => typeof test_RecursiveObject]>; | ||
readonly number: TreeNodeSchema<"com.fluidframework.leaf.number", NodeKind.Leaf, number, number>; | ||
}> & WithType<"Test Recursive Domain.testObject">, object & InsertableObjectFromSchemaRecord< { | ||
readonly recursive: FieldSchema<import("./schemaTypes.js").FieldKind.Optional, readonly [() => typeof test_RecursiveObject]>; | ||
readonly number: TreeNodeSchema<"com.fluidframework.leaf.number", NodeKind.Leaf, number, number>; | ||
export const test_RecursiveObject_base: TreeNodeSchemaClass_2<"Test Recursive Domain.testObject", NodeKind.Object, object & TreeNode_2 & ObjectFromSchemaRecord_2< { | ||
readonly recursive: FieldSchema_2<FieldKind_2.Optional, readonly [() => typeof test_RecursiveObject]>; | ||
readonly number: TreeNodeSchema_2<"com.fluidframework.leaf.number", NodeKind.Leaf, number, number>; | ||
}> & WithType_2<"Test Recursive Domain.testObject">, object & InsertableObjectFromSchemaRecord_2< { | ||
readonly recursive: FieldSchema_2<FieldKind_2.Optional, readonly [() => typeof test_RecursiveObject]>; | ||
readonly number: TreeNodeSchema_2<"com.fluidframework.leaf.number", NodeKind.Leaf, number, number>; | ||
}>, true>; | ||
|
||
// @internal | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,17 +12,19 @@ | |
"author": "Microsoft and contributors", | ||
"sideEffects": false, | ||
"main": "dist/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. "type" should be set (prefer "module" with esm as first class.) 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. Agree on removing the main/module field ultimately, but I'd like to do it separately, and ideally for all packages in the release group. Also, are you sure that removing these is not a breaking change? I thought at least webpack used the module field, but haven't yet confirmed whether removing it breaks our own examples. That's the other (minor) reason I want to do it all at once - so I can make only one "breaking change" note. |
||
"module": "lib/index.js", | ||
"module": "lib/index.mjs", | ||
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. Should just be removed - module is not supported |
||
"types": "dist/index.d.ts", | ||
"scripts": { | ||
"api": "fluid-build . --task api", | ||
"api-extractor:commonjs": "api-extractor run --local", | ||
"api-extractor:esnext": "api-extractor run --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:genver": "gen-version", | ||
"build:esnext": "tsc-multi --config ../../../common/build/build-common/tsc-multi.esm.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. I think we should prefer ESM first general but at least for |
||
"build:test": "tsc --project ./src/test/tsconfig.json", | ||
"check:are-the-types-wrong": "attw --pack . --entrypoints .", | ||
"check:release-tags": "api-extractor run --local --config ./api-extractor-lint.json", | ||
|
@@ -125,8 +127,21 @@ | |
"prettier": "~3.0.3", | ||
"rimraf": "^4.4.0", | ||
"ts2esm": "^1.1.0", | ||
"tsc-multi": "^1.1.0", | ||
"typescript": "~5.1.6" | ||
}, | ||
"fluidBuild": { | ||
"tasks": { | ||
"build:docs": { | ||
"dependsOn": [ | ||
"...", | ||
"api-extractor:commonjs", | ||
"api-extractor:esnext" | ||
], | ||
"script": false | ||
} | ||
} | ||
}, | ||
"typeValidation": { | ||
"disabled": true, | ||
"broken": {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
*/ | ||
|
||
import { BTree } from "@tylerbu/sorted-btree-es6"; | ||
import { createEmitter, ISubscribable } from "../../events/index.js"; | ||
import { createEmitter, HasListeners, IEmitter, ISubscribable } from "../../events/index.js"; | ||
import { compareStrings } from "../../util/index.js"; | ||
import { | ||
StoredSchemaCollection, | ||
|
@@ -54,13 +54,17 @@ export interface MutableTreeStoredSchema extends TreeStoredSchemaSubscription { | |
apply(newSchema: TreeStoredSchema): void; | ||
} | ||
|
||
type eventsEmitterType = ISubscribable<SchemaEvents> & | ||
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. Some explicit types added where they were simple, to reduce the number of dynamic imports in |
||
IEmitter<SchemaEvents> & | ||
HasListeners<SchemaEvents>; | ||
|
||
/** | ||
* Mutable TreeStoredSchema repository. | ||
*/ | ||
export class TreeStoredSchemaRepository implements MutableTreeStoredSchema { | ||
protected nodeSchemaData: BTree<TreeNodeSchemaIdentifier, TreeNodeStoredSchema>; | ||
protected rootFieldSchemaData: TreeFieldStoredSchema; | ||
protected readonly events = createEmitter<SchemaEvents>(); | ||
protected readonly events: eventsEmitterType = createEmitter<SchemaEvents>(); | ||
|
||
/** | ||
* Copies in the provided schema. If `data` is an TreeStoredSchemaRepository, it will be cheap-cloned. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -277,27 +277,27 @@ export class SchemaBuilder< | |
/** | ||
* {@link leaf.number} | ||
*/ | ||
public readonly number = leaf.number; | ||
public readonly number: typeof leaf.number = leaf.number; | ||
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. Some explicit types added where they were simple, to reduce the number of dynamic imports in 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. We shouldn't have to make any source changes to get rollups and generate API reports. |
||
|
||
/** | ||
* {@link leaf.boolean} | ||
*/ | ||
public readonly boolean = leaf.boolean; | ||
public readonly boolean: typeof leaf.boolean = leaf.boolean; | ||
|
||
/** | ||
* {@link leaf.string} | ||
*/ | ||
public readonly string = leaf.string; | ||
public readonly string: typeof leaf.string = leaf.string; | ||
|
||
/** | ||
* {@link leaf.handle} | ||
*/ | ||
public readonly handle = leaf.handle; | ||
public readonly handle: typeof leaf.handle = leaf.handle; | ||
|
||
/** | ||
* {@link leaf.null} | ||
*/ | ||
public readonly null = leaf.null; | ||
public readonly null: typeof leaf.null = leaf.null; | ||
|
||
/** | ||
* Function which can be used for its compile time side-effects to tweak the evaluation order of recursive types to make them compile. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,12 +20,13 @@ import { | |
referenceFreeFieldChangeRebaser, | ||
FieldKindWithEditor, | ||
} from "../modular-schema/index.js"; | ||
import { sequenceFieldChangeHandler } from "../sequence-field/index.js"; | ||
import { SequenceFieldEditor, sequenceFieldChangeHandler } from "../sequence-field/index.js"; | ||
import { | ||
noChangeCodecFamily, | ||
OptionalChangeset, | ||
optionalChangeHandler, | ||
optionalFieldEditor, | ||
OptionalFieldEditor, | ||
} from "../optional-field/index.js"; | ||
import { Multiplicity } from "../multiplicity.js"; | ||
|
||
|
@@ -59,16 +60,17 @@ const optionalIdentifier = "Optional"; | |
/** | ||
* 0 or 1 items. | ||
*/ | ||
export const optional = new FieldKindWithEditor( | ||
optionalIdentifier, | ||
Multiplicity.Optional, | ||
optionalChangeHandler, | ||
(types, other) => | ||
(other.kind.identifier === sequence.identifier || | ||
other.kind.identifier === optionalIdentifier) && | ||
allowsTreeSchemaIdentifierSuperset(types, other.types), | ||
new Set([]), | ||
); | ||
export const optional: FieldKindWithEditor<OptionalFieldEditor, Multiplicity.Optional, "Optional"> = | ||
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. Some explicit types added where they were simple, to reduce the number of dynamic imports in |
||
new FieldKindWithEditor( | ||
optionalIdentifier, | ||
Multiplicity.Optional, | ||
optionalChangeHandler, | ||
(types, other) => | ||
(other.kind.identifier === sequence.identifier || | ||
other.kind.identifier === optionalIdentifier) && | ||
allowsTreeSchemaIdentifierSuperset(types, other.types), | ||
new Set([]), | ||
); | ||
|
||
export const valueFieldEditor: ValueFieldEditor = { | ||
...optionalFieldEditor, | ||
|
@@ -104,16 +106,17 @@ const sequenceIdentifier = "Sequence"; | |
/** | ||
* 0 or more items. | ||
*/ | ||
export const sequence = new FieldKindWithEditor( | ||
sequenceIdentifier, | ||
Multiplicity.Sequence, | ||
sequenceFieldChangeHandler, | ||
(types, other) => | ||
other.kind.identifier === sequenceIdentifier && | ||
allowsTreeSchemaIdentifierSuperset(types, other.types), | ||
// TODO: add normalizer/importers for handling ops from other kinds. | ||
new Set([]), | ||
); | ||
export const sequence: FieldKindWithEditor<SequenceFieldEditor, Multiplicity.Sequence, "Sequence"> = | ||
new FieldKindWithEditor( | ||
sequenceIdentifier, | ||
Multiplicity.Sequence, | ||
sequenceFieldChangeHandler, | ||
(types, other) => | ||
other.kind.identifier === sequenceIdentifier && | ||
allowsTreeSchemaIdentifierSuperset(types, other.types), | ||
// TODO: add normalizer/importers for handling ops from other kinds. | ||
new Set([]), | ||
); | ||
|
||
const nodeKeyIdentifier = "NodeKey"; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ export { OptionalChangeset, RegisterId } from "./optionalFieldChangeTypes.js"; | |
export { | ||
optionalChangeHandler, | ||
optionalFieldEditor, | ||
OptionalFieldEditor, | ||
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. This was done so I could add explicit types to some things, following what sequence field already did in terms of exports. |
||
optionalChangeRebaser, | ||
optionalFieldIntoDelta, | ||
} from "./optionalField.js"; |
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 just be removed - module is not supported and never will be