-
Notifications
You must be signed in to change notification settings - Fork 1
Introduce WS tool to help publishing a new package #1227
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
base: main
Are you sure you want to change the base?
Conversation
|
…ing or encoding Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
somewhatabstract
left a comment
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.
This looks pretty good to me as a concept. I really dislike the AI generated code; lots of things not following our best practices. I've left some comments.
Main things;
- the unit tests should follow our best practices and conventions
- I really hate files that have tons of functions in them. It makes unnecessary decision points; where do I put my new function? Which file does it fit with? And it makes code discovery and workspace navigation harder, with cryptic filenames that don't necessarily describe their contents well, with files that end up being really huge and hard to navigate. It's so much easier to just have one file per function. Generally makes testing easier too and helps to make sure things that need tests have tests.
|
|
||
| describe("git", () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); |
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.
suggestion: Use resetAllMocks so that mocked implementations are also reset. This keeps the mocks around, but strips them of any configured returns or implementations.
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.
Turns out I don't need them at all because we reset them in our base Jest config. I removed this and all tests continue to pass.
Co-authored-by: Jeff Yates <jeff@khanacademy.org>
Co-authored-by: Jeff Yates <jeff@khanacademy.org>
|
@somewhatabstract I think I've addressed all your comments (except the verbose logging suggestion). Tagging you for one more review. Feel free to tag anyone else you think would be interested. |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
Looks like you have some lint errors |
| const tempDir = fs.mkdtempSync( | ||
| path.join(os.tmpdir(), `test-git-repo-${crypto.randomUUID()}`), | ||
| ); | ||
|
|
||
| execSync("git init", {cwd: tempDir}); | ||
| execSync(`git remote add origin "${remoteUrl}"`, {cwd: tempDir}); | ||
|
|
||
| tempDirs.push(tempDir); | ||
|
|
||
| return tempDir; |
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.
suggestion: This feels a little scary; making actual file system changes, but it's in temp so that's good.
I think that it perhaps needs to be a little more defensive. What happens if execSync throws?
| const tempDir = fs.mkdtempSync( | |
| path.join(os.tmpdir(), `test-git-repo-${crypto.randomUUID()}`), | |
| ); | |
| execSync("git init", {cwd: tempDir}); | |
| execSync(`git remote add origin "${remoteUrl}"`, {cwd: tempDir}); | |
| tempDirs.push(tempDir); | |
| return tempDir; | |
| const tempDir = fs.mkdtempSync( | |
| path.join(os.tmpdir(), `test-git-repo-${crypto.randomUUID()}`), | |
| ); | |
| tempDirs.push(tempDir); | |
| execSync("git init", {cwd: tempDir}); | |
| execSync(`git remote add origin "${remoteUrl}"`, {cwd: tempDir}); | |
| return tempDir; |
| const dirsToDelete = tempDirs.splice(0, tempDirs.length); | ||
| dirsToDelete.forEach((dir) => fs.rmSync(dir, {recursive: true})); |
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.
suggestion: I think that this needs to be more defensize. I would also use a for...of loop rather than the forEach, but that's a stylistic nit.
| const dirsToDelete = tempDirs.splice(0, tempDirs.length); | |
| dirsToDelete.forEach((dir) => fs.rmSync(dir, {recursive: true})); | |
| const dirsToDelete = tempDirs.splice(0, tempDirs.length); | |
| for (const dir of dirsToDelete) { | |
| try { | |
| fs.rmSync(dir, {recursive: true}); | |
| } catch { | |
| /* ignore */ | |
| } | |
| } |
somewhatabstract
left a comment
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.
Mostly looks good. Some housekeeping and related things:
- I don't think this package should export anything. It should just have a the
binand that's it. - The tool implementation should just be in the bin file, it doesn't need to be elsewhere
- The function docs aren't following our conventions
| ); | ||
|
|
||
| // Act | ||
| const act = () => detectGitRepoOriginUrl(nonExistantWorkingDir); |
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.
nit: In general, since React testing uses act for a specific thing in its domain, we tend to use underTest for this across our frontend code. Not a big deal, though.
| const dirsToDelete = tempDirs.splice(0, tempDirs.length); | ||
| dirsToDelete.forEach((dir) => fs.rmSync(dir, {recursive: true})); |
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.
suggestion: I think this needs to be more defensive.
| const dirsToDelete = tempDirs.splice(0, tempDirs.length); | |
| dirsToDelete.forEach((dir) => fs.rmSync(dir, {recursive: true})); | |
| const dirsToDelete = tempDirs.splice(0, tempDirs.length); | |
| for (const dir of dirsToDelete) { | |
| try { | |
| fs.rmSync(dir, {recursive: true}); | |
| } catch { | |
| /* ignore */ | |
| } | |
| } |
| }); | ||
|
|
||
| async function makeTempDirAndWritePackageFiles() { | ||
| // Arrange |
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.
suggestion: Unnecessary // Arrange.
| // Arrange |
| const localName = crypto.randomUUID(); | ||
| const packageName = `@khan/${localName}`; | ||
|
|
||
| // Act |
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.
suggestion: Unnecessary // Act
| // Act |
| const tempDir = fs.mkdtempSync( | ||
| path.join(os.tmpdir(), "write-package-files-tests-"), | ||
| ); |
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.
suggestion: This is not getting added to the tempDirs array for cleanup.
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.
suggestion: It's unnecessary to have the code live in a separate file here, I think. Just have it all in this file.
| /** | ||
| * The parsed arguments from the command line. | ||
| */ | ||
| interface ParsedArgs { |
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.
suggestion: Missing export?
| * | ||
| * If invalid/incompatible parameters were provided, prints usage info. | ||
| */ | ||
| export function parseArgs(): ParsedArgs { |
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.
suggestion: Not importing the type
| * Generates and writes the placeholder package files using tempDir as the | ||
| * package working directory. |
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.
suggestion: Our convention is to use the imperative in a single line first, and then a broader description. Also, should document params and return.
| * Generates and writes the placeholder package files using tempDir as the | |
| * package working directory. | |
| * Generate and write the placeholder package files. | |
| * | |
| * This generated and writes the placeholder package files using | |
| * the given dir as the package working directory. | |
| * | |
| * @param tempDir The package working directory. | |
| * @param packageName The package name. | |
| * @param repoName The repo name to use for the package.json. | |
| * @returns A promise that resolves when the package files are written. |
| // Allow console.log in tool packages | ||
| "packages/wonder-stuff-tool-*/src/**/*.ts", |
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.
praise: Nice.
Summary:
This PR introduces a new tool to help with publishing a new package. We (Khan Academy) have moved to Trusted Publishing for all npm package publishing but one can't configure Trusted Publishing until the package exists on npmjs.com (do you see a problem here? ;)
To help with this, this tool guides the author through publishing a placeholder package. Once the placeholder has been published, we can configure it and from then on publish via Trusted Publishing.
Issue: LEMS-3681
Test plan:
I tested this by running
pnpm buildin the repo on this branch.Then I switched to
/tmpand ran the following commands (note that it failed to publish because I gave it a bogus token). I'm hesitant to use a real token as I don't want to pollute npmjs with dummy packages.