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

Make ts-common-config.json extends tsconfig.base.json #23532

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

CraigMacomber
Copy link
Contributor

@CraigMacomber CraigMacomber commented Jan 13, 2025

Description

Make deprecated common/build/build-common/ts-common-config.json extend common/build/build-common/tsconfig.base.json.

This avoids duplicating things like selecting the target.

As noted in common/build/build-common/README.md, tsconfig.base.json is the "base config contains defaults that all packages within the repo should use as a baseline".
This change causes the packages which use ts-common-config.json to follow this pattern, allowing tsconfig.base.json to better fill its role as the actual base configuration which provides a single source of truth for some of our build options.

Reviewer Guidance

The review process is outlined on this wiki page.

@Copilot Copilot bot review requested due to automatic review settings January 13, 2025 19:09

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • common/build/build-common/ts-common-config.json: Language not supported
@github-actions github-actions bot added area: build Build related issues base: main PRs targeted against main branch and removed area: build Build related issues labels Jan 13, 2025
@CraigMacomber CraigMacomber mentioned this pull request Jan 13, 2025
Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

I am assuming the intent here is to highlight how this differs from base and make it so that changes to base reach more even if they are out of date on config updates. I'd note that in the description.
Additional clean up that looks like it could be done immediately is to delete tsconfig.test.json, tsconfig.esm.json, and tsconfig.esm-only.json.

common/build/build-common/ts-common-config.json Outdated Show resolved Hide resolved
@@ -1,20 +1,13 @@
{
// This configuration is deprecated and ussages should be migrated to other options.
// The "common" in this file name means either that this is commonjs, or that this is a shared config since both are true.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would strike this comment. I am confident that the "common" in the name means it is shared. But that isn't useful really since it is deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As is this comment conveys more useful information (implying why the other configs don't follow this pattern of calling themselves common which might be confusing if we simply point out common means shared here, as that applies to every config in this package)

Copy link
Contributor

Choose a reason for hiding this comment

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

What I read, when I read the comment, is that the author doesn't really know the meaning, but happily this file currently satisfies two things.
The aspect that seems unfortunate is that we don't want this to be common/shared. Just feels a bit like it degrades the don't use this message from the line above. But I'm okay with keeping it.

@github-actions github-actions bot added the area: build Build related issues label Jan 13, 2025
@CraigMacomber
Copy link
Contributor Author

I am assuming the intent here is to highlight how this differs from base and make it so that changes to base reach more even if they are out of date on config updates. I'd note that in the description.

Yes. Done.

Additional clean up that looks like it could be done immediately is to delete tsconfig.test.json, tsconfig.esm.json, and tsconfig.esm-only.json.

That seems likely. Might be good to update packages/framework/client-logger/fluid-telemetry/tsconfig.json and remove tsconfig.cjs.json at the same time (that or update the docs to move it out of "Legacy tsconfig"). Do you want to make that change?

@tylerbutler
Copy link
Member

Note that this change alone will not affect some packages, including those in /common and /server, because those packages extend from the version in their build-common dependency, @fluidframework/build-common/ts-common-config.json. I think that's probably preferred but it will require more changes to switch the other packages to use relative paths instead of the dependency version.

@CraigMacomber CraigMacomber merged commit fd30517 into microsoft:main Jan 13, 2025
46 checks passed
@CraigMacomber CraigMacomber deleted the commonBase branch January 13, 2025 20:43
@jason-ha
Copy link
Contributor

That seems likely. Might be good to update packages/framework/client-logger/fluid-telemetry/tsconfig.json and remove tsconfig.cjs.json at the same time (that or update the docs to move it out of "Legacy tsconfig"). Do you want to make that change?

#23534

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.

3 participants