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

feat(build-tools): Add generate:node10Entrypoints command #22937

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sonalideshpandemsft
Copy link
Contributor

@sonalideshpandemsft sonalideshpandemsft commented Oct 30, 2024

This PR add a new command to generate node10 type declaration entrypoints. Most of the node10 code is extracted from generate entrypoint command.

@github-actions 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 30, 2024
@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label Oct 30, 2024
@sonalideshpandemsft sonalideshpandemsft marked this pull request as ready for review October 30, 2024 17:47
@@ -60,3 +60,4 @@ export {
} from "./release.js";
export { LayerGraph } from "./layerGraph.js";
export { type Handler, policyHandlers } from "./repoPolicyCheck/index.js";
export { Node10CompatExportData } from "./packageExports.js";
Copy link
Member

Choose a reason for hiding this comment

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

As in the other PR, just import directly from the submodules and remove the export here.

Comment on lines +110 to +114
// Reads and parses the `tsconfig.json` file in the current directory.
export async function readTsConfig(): Promise<TsConfigJson> {
const tsConfigContent = await fs.readFile("./tsconfig.json", { encoding: "utf8" });
return JSON5.parse(tsConfigContent);
}
Copy link
Member

Choose a reason for hiding this comment

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

Since you need these util functions in both commands now, put them somewhere in library.


`;

async function generateNode10TypeEntrypoints(
Copy link
Member

Choose a reason for hiding this comment

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

Is this the same as the exported function from library/commands/? Any reason not to use the exported function?

Copy link
Contributor Author

@sonalideshpandemsft sonalideshpandemsft Oct 30, 2024

Choose a reason for hiding this comment

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

My understanding is generateEntrypoints will be deprecated and eventually removed once sourceEntrypoints is merged. It doesn't seem appropriate for the Node 10 function to be included under generateSourceEntrypoints, and I believe it would be better to isolate Node 10 functions since there are not many of them.

A minor difference is that the header varies, while the rest remains the same.

Comment on lines +47 to +51
export function mapExportPathsFromPackage(
packageJson: PackageJson,
emitDeclarationOnly: boolean,
logger?: Logger,
): Map<string, Node10CompatExportData> {
Copy link
Member

Choose a reason for hiding this comment

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

Should be replaceable with a shared function, maybe even the one in generate:typetests.

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.

2 participants