feat(build-tools): Add generate:source:entrypoints command by sonalideshpandemsft · Pull Request #22895 · microsoft/FluidFramework

sonalideshpandemsft · 2024-10-24T23:39:45Z

Original PR: https://github.com/microsoft/FluidFramework/pull/22872/files.

The original PR generates source entry points for the odsp-client and datastore-definitions package.

This PR includes only the build-tools changes from the original PR. It introduces a new command, GenerateSourceEntrypointsCommand, which generates source entry points under src/entrypoints.

Once this PR is merged, there will be three additional PRs:

A new command to generate node10 compat entrypoints: feat(build-tools): Add generate:node10Entrypoints command #22937
An update to the build tools to the new version that includes these changes.
PR to update the Fluid build configuration and package.json: Update fluid build config #22896

…ild-tools

tylerbutler

My biggest concerns are the location of the command (it should be in the commands folder) and the added complexity to the build process. How will fluid-build handle these new tasks? How many more tasks will this introduce? We can address those in other PRs but I worry that this will complicate and/or slow down the build quite a bit.

Also is there a reason this command isn't a PackageCommand? It seems like a lot of the boilerplate code could be removed or simplified that way. You're always operating at the package level so the json parsing etc. could be done for you. Look at generate:typetests for an example. You can still have a per-package build task (again typetests are an example).

tylerbutler · 2024-10-25T19:05:14Z

+	mapSrcApiTagLevelToOutput: Map<ApiTag, ExportData>;
+} {
+	let emitDeclarationOnly: boolean = false;
+	if (tsconfig.compilerOptions?.emitDeclarationOnly !== undefined) {


Why is this needed?

If emitDeclarationOnly is set to true in the tsconfig, the types field should be referenced, as the default field may not be present. For example, datastore-definitions only emits type declarations.

https://github.com/microsoft/FluidFramework/blob/main/packages/runtime/datastore-definitions/package.json#L18
https://github.com/microsoft/FluidFramework/blob/main/packages/runtime/datastore-definitions/tsconfig.json#L8

I'm confused - what is driving the decision of what files to create and what files to read as input? I thought it was using the index.ts file to decide what the API levels in use are. Is it reading the package.json exports to determine that? Seems like there should be one input - the list of all exports. Why does the types or default field matter?

tylerbutler · 2024-10-25T19:10:41Z

+ * @param logger - optional Logger
+ * @returns object with mapKeyToOutput.
+ */
+export function queryDefaultResolutionPathsFromPackageExports<TOutKey>(


This function seems like it is replicating the exports resolution algorithm. We can just use that resolution logic directly. See use of resolve.imports elsewhere.

Related to an earlier comment - it's confusing me when we read from the exports field if we are also reading from index.ts to find the exports. Maybe a high level bulleted list of the steps would help? I thought it was:

Open the file passed into the CLI to find the full list of exports.

Determine what API levels are exported using the data from the previous step.

Create a TS file with the exports for the appropriate level.

tylerbutler · 2024-10-25T19:11:39Z

+ * @param emitDeclarationOnly -If true, "types" exports are considered.
+ * @returns matching export data and matching query entry value as outKey property
+ */
+function findDefaultPathsMatching<TOutKey>(


This should definitely be replaceable with resolve.imports.

jason-ha

I wasn't able to get through everything. But here are some comments.

jason-ha

There are two more pieces missing to make this a replacement for the existing code:

[previously noted] needs a task helper - see build-tools/packages/build-tools/src/fluidBuild/tasks/taskFactory.ts with "flub generate entrypoints": GenerateEntrypointsTask,
the output files need added into build database smarts: see build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildDatabase.ts's flubOutput that looks for "flub generate entrypoints". It looks like it needs a new case for "flub generate sourceEntrypoints".

The prior node10 change is probably missing similar. I am guessing node10 stuff can skip item 2 as we don't use those outputs ourselves.

jason-ha · 2024-11-05T20:52:21Z

 * (as opposed to a separate \@types package).
 */
-export function getTypesPathFromPackage(
+export function getExportPathFromPackage(


There is a lot left in here that is still "types" specific. There are comments and error messages and variable names.
I love reuse, but it needs cleaned up.

Note: I still see lots of "types" strings in this code.

jason-ha · 2024-11-05T20:55:05Z

+export function getExportPathFromPackage(
 	packageJson: PackageJson,
 	level: ApiLevel,
+	conditions: string[],


I don't think a generic array of strings is okay here. ["types"] is definitely okay. Looks like you tested some cases with ["default"]. So you could accept those specifically.
Curious if [] also works in place of ["default"].

yes, empty array falls back to default

Do update the typing to reflect the values that are okay.

jason-ha · 2024-11-05T21:04:35Z

+	// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+	const { rootDir, outDir, emitDeclarationOnly } = compilerOptions;


So many unsafe assignment disables - needs some comments

jason-ha · 2024-11-05T21:14:34Z

+		log.info(`\tGenerating ${outFile}`);
+		const sourceFile = project.createSourceFile(outFile, undefined, {


This logic should have some lines to create the target directory if it doesn't exist.
(It is getting by since current use expects tsconfig next to the target source location.)

I guess I have seen this create an output directory if one does not exist irrespective of tsconfig

jason-ha · 2024-11-05T21:46:07Z

+	}
+
+	// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+	const { rootDir, outDir, emitDeclarationOnly } = compilerOptions;


If we need emitDeclarationOnly (alternative would be to retry with "types" when "default" gives us nothing), then we really want tsconfig extends processing. (I commented, I think, in the usage example PR that it is not good to duplicate settings. It is going to get messed up by humans.)
I expect that rootDir and outDir are usually present based on how we usually write things and not in danger.

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildDatabase.ts's tscOutput contains all the smarts for accurately deriving tsc options including rootDir and outDir. (You would likely give tscUtils.parseCommandLine fake command line tsc --project <tsconfigPath> to get results.)

Reading emitDeclarationOnly simplifies concluding if the package is types only. resolve.exports have options to first check default and then types but that makes reading types only value difficult/messy.

…ild-tools

jason-ha · 2024-11-25T07:41:43Z

 	const outDir = options.outDir ?? ".";
+
 	const inputRegex = /(?:\.d)?(\.[cm]?ts)$/;


not an issue - just noticed so calling it out - intentional added blank line here?

jason-ha · 2024-11-25T07:44:14Z

+export class GenerateSourceEntrypointsTask extends TscDependentTask {
+	protected get configFileFullPaths() {
+		// Add package.json, which tsc should also depend on, but currently doesn't.
+		return [this.node.pkg.packageJsonFileName];
+	}
+
+	protected async getToolVersion() {
+		return getInstalledPackageVersion("@fluid-tools/build-cli", this.node.pkg.directory);
+	}
+}


How does this handle its output files? Looks like if we deleted one it wouldn't impact this task's sense of being complete, i.e. it wouldn't rerun using fluib-build.
(The other task took a shortcut for specifically specifying output files because there was nothing else in the system that depended on them but was still a problem if someone altered the output expect task to rerun on rebuild.)

jason-ha · 2024-11-25T08:00:02Z

 	log: Logger,
 	fallbackEntrypoint?: ApiLevel,
 ): { typesPath: string; entrypointUsed: ApiLevel } {
+	const getTypesPath = (level: ApiLevel): string | undefined => {


Prefer function getTypesPath(level: ApiLevel): string | undefined { to get named function.

jason-ha · 2024-11-25T08:44:11Z

+ * @param conditions - Export conditions to apply. Fallback to `default` if no condition is passed
+ * @returns A package relative path.
 */
-export function getTypesPathFromPackage(
+export function getExportPathFromPackage(


Use of this seems incorrect (fallible). When I looked at this a while back it didn't feel familiar, and something felt off. We can get away with using this for node10 and probably for type tests. Node10 has narrow "conditions". Typetests are also narrowly focused on an entrypoint and just types.
Some packages have conditions that are beyond "default" and "types". That is why the prior version of things does not use resolve.exports package which requires you to know the conditions. Instead, we want to find all of the paths under an export given the generation criteria and get their conditions. It was queryTypesResolutionPathsFromPackageExports that I was expecting a generic form of or to be adapted away from strictly for "types" conditions. (Would have matched the original command.)
Probably this pattern is why some other cases seem hard to account for - like stopping automatic generation of a file temporarily.
I need to think about this.

microsoft-github-policy-service · 2025-01-24T14:17:56Z

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

sonalideshpandemsft added 12 commits October 18, 2024 22:05

wip

c985bfe

wip: generate src files

eabda1f

wip

870acd6

wip

4bb4760

Fix tsconfig parsing

d88d955

wip

92f4074

Merge remote-tracking branch 'origin' into fix-vscode-auto-imports

038c8d8

update default paths

49e9670

wip

0f8ba65

Merge branch 'main' into fix-vscode-auto-imports

00595f0

add ignore pattern for eslint

23c386e

include build-tools changes

eff4e9f

github-actions Bot added base: main PRs targeted against main branch area: build Build related issues dependencies Pull requests that update a dependency file labels Oct 24, 2024

sonalideshpandemsft mentioned this pull request Oct 24, 2024

Update fluid build config #22896

Closed

revert build-tools changes

0e41310

sonalideshpandemsft force-pushed the fix-vscode-auto-imports-build-tools branch from d5d683b to ade66c5 Compare October 25, 2024 17:23

Merge remote-tracking branch 'origin' into fix-vscode-auto-imports-bu…

8dbad63

…ild-tools

sonalideshpandemsft force-pushed the fix-vscode-auto-imports-build-tools branch from ade66c5 to 8dbad63 Compare October 25, 2024 17:28

reset lockfile

7c56477

github-actions Bot removed the dependencies Pull requests that update a dependency file label Oct 25, 2024

add docs

fbe8d00

sonalideshpandemsft force-pushed the fix-vscode-auto-imports-build-tools branch from cf9cd32 to fbe8d00 Compare October 25, 2024 18:38

sonalideshpandemsft changed the title ~~Add tool to generate source and node10 compat entrypoints~~ Add generate:source:entrypoints command and node10 compat entrypoints Oct 25, 2024

sonalideshpandemsft marked this pull request as ready for review October 25, 2024 18:46

sonalideshpandemsft requested review from jason-ha and tylerbutler October 25, 2024 18:47

tylerbutler reviewed Oct 25, 2024

View reviewed changes

feedback wip

86c7bb6

remove quotes

8b9be0b

jason-ha reviewed Nov 2, 2024

View reviewed changes

Comment thread build-tools/packages/build-cli/src/library/packageExports.ts Outdated

jason-ha reviewed Nov 2, 2024

View reviewed changes

Comment thread build-tools/packages/build-cli/src/library/packageExports.ts Outdated

jason-ha reviewed Nov 2, 2024

View reviewed changes

Comment thread build-tools/packages/build-cli/src/commands/generate/sourceEntrypoints.ts Outdated

jason-ha reviewed Nov 2, 2024

View reviewed changes

Comment thread build-tools/packages/build-cli/src/commands/generate/sourceEntrypoints.ts Outdated

feedback

6f3d35b

jason-ha reviewed Nov 5, 2024

View reviewed changes

sonalideshpandemsft force-pushed the fix-vscode-auto-imports-build-tools branch 2 times, most recently from 9290864 to bd2ceb1 Compare November 5, 2024 15:40

feedback

cb1a30f

sonalideshpandemsft force-pushed the fix-vscode-auto-imports-build-tools branch from bd2ceb1 to cb1a30f Compare November 5, 2024 15:43

Merge branch 'main' into fix-vscode-auto-imports-build-tools

998c4a0

jason-ha reviewed Nov 5, 2024

View reviewed changes

sonalideshpandemsft added 2 commits November 19, 2024 10:31

feedback wip

9f12ae1

Merge remote-tracking branch 'origin' into fix-vscode-auto-imports-bu…

bd6c5db

…ild-tools

sonalideshpandemsft force-pushed the fix-vscode-auto-imports-build-tools branch from 2df78e3 to bd6c5db Compare November 19, 2024 13:04

sonalideshpandemsft added 3 commits November 21, 2024 12:20

wip

c5bb0f6

wip

df20c12

Merge remote-tracking branch 'origin' into fix-vscode-auto-imports-bu…

6f389e3

…ild-tools

sonalideshpandemsft force-pushed the fix-vscode-auto-imports-build-tools branch from 39f0a6e to 6f389e3 Compare November 22, 2024 20:20

sonalideshpandemsft added 3 commits November 23, 2024 09:42

test

a6d4063

Merge remote-tracking branch 'origin' into test-vscode

10b5303

Merge remote-tracking branch 'origin' into fix-vscode-auto-imports-bu…

59bd3fa

…ild-tools

sonalideshpandemsft force-pushed the fix-vscode-auto-imports-build-tools branch from b08ccf1 to a27ab9a Compare November 24, 2024 14:27

Merge branch 'test-vscode' into fix-vscode-auto-imports-build-tools

edfcb69

sonalideshpandemsft force-pushed the fix-vscode-auto-imports-build-tools branch from a27ab9a to edfcb69 Compare November 24, 2024 14:37

jason-ha reviewed Nov 25, 2024

View reviewed changes

microsoft-github-policy-service Bot added the status: stale label Jan 24, 2025

microsoft-github-policy-service Bot closed this Feb 2, 2025

		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
		const { rootDir, outDir, emitDeclarationOnly } = compilerOptions;

		log.info(`\tGenerating ${outFile}`);
		const sourceFile = project.createSourceFile(outFile, undefined, {

		const outDir = options.outDir ?? ".";

		const inputRegex = /(?:\.d)?(\.[cm]?ts)$/;

Conversation

sonalideshpandemsft commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylerbutler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jason-ha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jason-ha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonalideshpandemsft Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonalideshpandemsft commented Oct 24, 2024 •

edited

Loading

sonalideshpandemsft Nov 21, 2024 •

edited

Loading