improvement(build-tools): fixes for ESM builds #19625
Conversation
github-actions
bot
added
area: build
github-actions
bot
added
the
dependencies
- Recognize more multi-command scripts in package.json Ideally we would use broken down scripts, but those result in additional dependecy complexies and don't offer simple atomicity. - Recognize tsc-multi.node16.cjs as using tsconfig.json when checking project references. There are no unit test for these change. The were manually tested against repo with the exclusions for fluid-build-tasks-tsc and npm-package-json-esm fully reverted.
+ update TscMultiTask for tsc-multi patch + update tsc-multi patch to have better tsconfig*.tsbuildinfo pattern
jason-ha
force-pushed
the
tool-updates-for-esm
branch
from
0af1edb
to
cdced90
Compare
| @@ -113,7 +113,7 @@ const testString: string[] = [ | |||
| * Generated by fluid-type-test-generator in @fluidframework/build-tools. | |||
| */ | |||
| import type * as old from "${previousPackageName}"; | |||
| import type * as current from "../../index"; | |||
| import type * as current from "../../index.js"; | |||
There was a problem hiding this comment.
No need to handle cjs/mjs here?
There was a problem hiding this comment.
I don't think /index would be found as /index.cjs or /index.mjs, which makes this a non-change [except maybe in the case that index was a folder].
If we are building tests with tsc-multi and renaming (pattern we are going away from), then this would be renamed. If building only the product code with tsc-multi and renaming, then I think this was already broken.
Nothing broken on my local system when this was run for client group.
build-tools/packages/build-tools/src/fluidBuild/tasks/leaf/tscTask.ts
Outdated
Show resolved
Hide resolved
build-tools/packages/build-tools/src/repoPolicyCheck/handlers/fluidBuildTasks.ts
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| return undefined; | ||
| return bestScript.script; |
There was a problem hiding this comment.
Seams reasonablish... I vaguely understand that FluidBuild is looking for tasks containing 'tsc' to build/check dependencies and assume the complication is that we're now using 'tsc && copyfiles'.
From your comments on the overall PR, I'm guessing the alternative would be to break 'tsc && copyfiles' into separate scripts/tasks with more scripts/tasks to tie them together.
I'm happy to trust your judgement on the tradeoffs.
There was a problem hiding this comment.
There was already some support for && in the dependencies but it didn't extend to the tsc searches.
Once I added support for && to that, I found that there were other packages with tsc commands embeded. packages/dd/matrix bench:profile is the notable one. AFAIK it is actually outside the general build:fast support and doesn't actually build like the cases this is trying to find.
I had added this comment to the description but didn't find a good place to have it in code:
- Recognize more multi-command scripts in package.json
Ideally, we would use broken down scripts, but
those result in additional dependency complexities
and don't offer simple atomicity.
moduleexpectation for ESMIdeally, we would use broken down scripts, but
those result in additional dependency complexities
and don't offer simple atomicity.
tsconfig.json when checking project references.
There are no unit test for these change. The were
manually tested against repo with the exclusions
for fluid-build-tasks-tsc and npm-package-json-esm
fully reverted.