-
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
improvement(build-tools): Convert build-cli to ESM #20972
Conversation
@@ -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", |
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.
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?
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.
How do you connect your local build tools to client for testing?
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.
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.
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.
It is the change in recognized module type for build-cli that is tripping up the policy dependency checking.
- We have a global task config that ^tsc is done for dependencies listed when running tsc.
- 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. - 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
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.
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+.
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.
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.
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.
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.
jssm is, in fact, compatible with node16 resolution. i've used it that way every day for years |
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 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 |
@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:
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). |
@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. |
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: