Skip to content
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

improvement(build-tools): Convert build-cli to ESM #20972

Merged
merged 71 commits into from
May 28, 2024

Conversation

tylerbutler
Copy link
Member

@tylerbutler tylerbutler commented May 3, 2024

Updates the build-cli project to be ESM instead of CJS. For the oclif-related changes I followed this guide: https://oclif.io/docs/esm/#migrating-a-cjs-plugin-to-esm

Summary of changes:

  • Ran ts2esm to add extensions to source file import statements.
  • Upgraded chalk, fs-extra, and node-fetch to more recent versions with better ESM support.
  • jssm's types aren't set up quite right for TypeScript to resolve them when using Node16/NodeNext module resolution, so I patched in support. I opened jssm does not work with TypeScript's Node16/NodeNext/Bundler moduleResolution StoneCypher/fsl#1295 to discuss options for upstreaming the changes.
    • Update: I've modified the jssm patch to be the latest version (5.98.2), and I've limited the patched files to package.json, rollup configs, and the two new rolled up declaration files. Critically, this means the patch has no runtime changes. The only changes are in the types and config files. This should make it almost impossible for there to have been runtime bugs introduced by these changes.
  • Updated the install build tools step in the CI workflows to use npm link to make flub globally available. I removed the hack shim we only used in CI.
  • Updated the syncpack config to account for new dependencies.
  • Regenerated the packlists. This should be done as a part of the build but it isn't yet so I updated them opportunistically.
  • Renamed the dangerfile to .cts so it's always CommonJS like it used to be. Seemed the safest approach. If we want it to be ESM we can make that change separately.

@github-actions github-actions bot added area: build Build related issues base: main PRs targeted against main branch labels May 3, 2024
@@ -206,6 +206,10 @@ module.exports = {
// TODO: AB#7630 uses lint only ts projects for coverage which don't have representative tsc scripts
"^packages/tools/fluid-runner/package.json",
],
"fluid-build-tasks-tsc": [
// False positive?
"^packages/test/test-end-to-end-tests/package.json",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this exclusion, I get the pollowing policy errors:

file failed the "fluid-build-tasks-tsc" policy: packages/test/test-end-to-end-tests/package.json
'build:test:esm' task is missing the following dependency: 
        @fluid-tools/build-cli#build:test or @fluid-tools/build-cli#tsc

Very strange that it's looking for deps on build-cli? @jason-ha Do you know what's happening here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you connect your local build tools to client for testing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually don't, but when I do, I use pnpm.overrides. SOmething like:

"@fluid-tools/build-cli": "link:./build-tools/packages/build-cli",
"@fluidframework/build-tools": "link:./build-tools/packages/build-tools"

But the policy failure is happening in CI, too, so seems to be unrelated to my local setup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the change in recognized module type for build-cli that is tripping up the policy dependency checking.

  1. We have a global task config that ^tsc is done for dependencies listed when running tsc.
  2. A tsc script is now recognized for CommonJS or ESM. The policy roughly says is there is a dependency with a matching module type then the task dependency must be declared.
  3. With your change build-cli tsc switched from CommonJS to ESM.

The dependency policy said the ESM of all of the packages that have build-cli listed must depend on build-cli tsc (or build:test since is doesn't know which one is the product build). The system does not have precise knowledge to distinguish library (import) dependencies versus execution dependencies. The post-build policy check I would like to see will be able to tell the difference.
So, for the approximate check we do now, I have added logic not to require dependencies across release groups. See #21238

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; I updated the comment with the link to your PR and noted that the override can be removed once client uses build-tools 0.39+.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Only one package needed override? When I was using locally built build-tools, I was getting reports for just about every package. If it is isolated, then client group isn't using the build-tools that restores deps checks for scripts that are not just "tsc". That would mean instead of config suppression you could set build-cli "tsc" to "tsc --project tsconfig.json", which is the common pattern we ended up using. I am good with either.
Going to unresolve just for visibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried changing build-cli's tsc script to be tsc --project tsconfig.json but no change, so I left the config exclusion and comment.

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label May 22, 2024
@StoneCypher
Copy link

jssm is, in fact, compatible with node16 resolution. i've used it that way every day for years

@StoneCypher
Copy link

this is very confusing because one of the underlying changes you're requesting (adding extensions to the filenames) is something that the microsoft typescript team has been fighting actively against for almost a decade

They've even admitted in the open that the rules are incompatible between them and regular ES modules, and this in turn breaks me, because I have legacy to support

Moreover there doesn't seem to be any documentation anywhere specifying what is "correct" about the types. What am I supposed to do? I have all the .d.ts everywhere already. I'd rather just add whatever field is necessary in package than start making broad changes to the fundamental structure of the project which break it in deno, rome, and bun

what i'm doing is generally considered the community golden standard: compile down to es5 then compile back upwards to es6 as well as iife-for-non-module-browser, then release in all formats for broad compatibility

i don't know what this "are the types wrong" tool is, but i know that if someone's going to write a tool saying something is incorrect, the tool becomes valuable on the day that it says why something is incorrect

a tool that tells me something is incorrect but doesn't tell me what correct is leaves me confused whether a problem even really exists

i'd love to fix this but i have no genuine understanding of the defect, and there are too many things in the PR i was offered which are unrelated to the problem, which are against what the typescript team says you're supposed to do in the manual, for me to feel safe merging it

@mhsdef suggested that the problem might be the TS 4.7 subdivision of types thing as described here

if you were to give me a project exhibiting the problem, i could test that as a potential repair

i do want to help and it's very flattering to be potentially included in a tool like this

@tylerbutler
Copy link
Member Author

tylerbutler commented May 23, 2024

@StoneCypher thanks so much for chiming in here. I realized that I've conflated a couple of related but separate issues. That is: issue 1 is that TypeScript can't find types and issue 2 is that when in Node16 mode, Typescript has compilation errors because of the "bare imports" in the source.

the changes in my branch were really about issue 2. Sorry for being so confusing. I think issue 2 is arguably a non issue for jssm because you probably could use modeuleResokution: Bundler which should make issue 2 go away, and it sounds like you supply node with bundled code anyway, so that seems like an accurate setting too.

Issue 1 is really the more relevant one. The problems you've noted with TS are exactly what we've been dealing with for the past few months as we converted all of our packages to dual format ESM and CJS. Fundamentally they seem to be opposed to tsc being capable of "dual emit." I share your frustration.

I'll do some more testing tomorrow, but I think the fix for jssm looks like:

  • add types entries to the export map
  • produce a types file for each JS file, since TS expects that

I'll dig into it more tomorrow and share what I find on the issue I opened in the fsl repo.

Thanks again for your help. I think these changes will really help more folks use jssm (which is awesome, btw).

@tylerbutler
Copy link
Member Author

i don't know what this "are the types wrong" tool is, but i know that if someone's going to write a tool saying something is incorrect, the tool becomes valuable on the day that it says why something is incorrect

a tool that tells me something is incorrect but doesn't tell me what correct is leaves me confused whether a problem even really exists

@StoneCypher Sorry, I should have provided a lot more context and info instead of just a link to the tool. It does offer quite detailed debugging info; I'll share more tomorrow as I explore the changes I mentioned earlier. It's really most handy for testing types-related export map changes without a test project.

@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label May 23, 2024
@tylerbutler tylerbutler merged commit 6a415d1 into microsoft:main May 28, 2024
63 checks passed
@tylerbutler tylerbutler deleted the bt-esm-fresh branch May 28, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Build related issues base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants