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(codegen): add a default prepack script #479

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

adamthom-amzn
Copy link
Contributor

Description of changes:
Doing a clean build in prepack works for both package managers and makes a
simple process of codegen && (npm|yarn) publish work out of the box. Also
adds an "omitPrepack" configuration option to allow the SDK to disable this
script for backwards compatibility.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -68,6 +68,10 @@ static void writePackageJson(
node = node.withMember("private", true);
}

if (settings.isOmitPrepack()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the SDK doesn't need it, I think we should not provide this option.

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

The change makes sense, but I'm not a big fan of the name prepack. We don't have a pack script, so the pre hook doesn't make a lot of sense. pack is a overloaded verb in JS ecosystem. What does it mean?

@adamthom-amzn
Copy link
Contributor Author

It's automatically run as a part of npm publish and it's recommended by yarn as a place to do transpilation before pack. So you don't have to define pack at all - it's a built-in part of the lifecycle of both package managers.

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Since we already defined a files in the package.json, which is an allowlist of all the paths to be pulished, do we still need to call the clean in the script?

@AllanZhengYP AllanZhengYP self-requested a review January 12, 2022 01:32
@adamthom-amzn
Copy link
Contributor Author

Since we already defined a files in the package.json, which is an allowlist of all the paths to be pulished, do we still need to call the clean in the script?

Wanted to make sure that npm publish didn't pick up a JavaScript file in dist for a TypeScript file that had been removed from the input.

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Just realize TSC doesn't remove the JS files from previous builds unless with --clean: microsoft/TypeScript#30602 (comment)

Ship it!

Doing a clean build in prepack works for both package managers and makes a
simple process of codegen && (npm|yarn) publish work out of the box.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants