feat: allow update function to trigger file deletion #130
Conversation
| @@ -54,7 +54,7 @@ export type UpdateFunctionFile = | |||
|
|
|||
| export type UpdateFunction = ( | |||
| file: UpdateFunctionFile | |||
| ) => string | File | null | Promise<string | File | null>; | |||
| ) => string | File | null | Symbol | Promise<string | File | null | Symbol>; | |||
There was a problem hiding this comment.
If the update function returns null it means no file changes.
If the update function return a symbol it triggers a file deletion.
There was a problem hiding this comment.
Is there a way to type the Symbol to ensure it is the actual deleteFile symbol?
There was a problem hiding this comment.
I don't know, I actually never worked with types for Symbols
src/create-tree.ts
Outdated
| @@ -20,6 +21,8 @@ export async function createTree( | |||
| for (const path of Object.keys(changes.files)) { | |||
| const value = changes.files[path]; | |||
|
|
|||
| // TODO use Symbol here which will result in a breaking change | |||
| // if (value === deleteFile) { | |||
There was a problem hiding this comment.
or we keep it as is at the drawback of having inconsistency between return values from an update function vs this code snipped
files: {
"file/to/delete.txt": null
}
There was a problem hiding this comment.
I'd rather go with a breaking change
There was a problem hiding this comment.
Done. Documented in the readme and commit message
| }); | ||
| continue; | ||
| } catch (error) { | ||
| // istanbul ignore next |
There was a problem hiding this comment.
I thought this is fine since I saw basically the same for other catch blocks, but please let me know if you would prefer a test case for this.
|
@gr2m this PR is ready for review.
|
README.md
Outdated
| const { | ||
| createPullRequest, | ||
| deleteFile, | ||
| } = require("octokit-plugin-create-pull-request"); |
There was a problem hiding this comment.
Seeing these two imports, I really don't like deleteFile. At first sight it suggests that it's a separate method to delete a file, don't you think?
Node.js has a convention to uses a k prefix for constants, see example: https://github.com/nodejs/node/blob/4bb06dbd0a87e5b2430780ddceec08015f8c496a/lib/_http_client.js#LL107C7-L107C13
Maybe we could use kDeleteFile? Or how about DELETE_FILE?
There was a problem hiding this comment.
Good shout. I changed it to DELETE_FILE which is well understood in our industry
BREAKING CHANGE: The meaning of `null` has changed!
Previously the following snippet was causing a file deletion.
files: {
"file/to/delete.txt": null
}
If you want to remain this behavior you must use the deleteFile Symbol
import { createPullRequest, deleteFile } from 'octokit-plugin-create-pull-request';
files: {
"file/to/delete.txt": deleteFile
}
If you want to trigger a file deletion from an update function, you can now do so by returning the deleteFile Symbol.
import { createPullRequest, deleteFile } from 'octokit-plugin-create-pull-request';
files: {
"file/to/delete.txt": ({ exists, encoding, content }) => {
const fileContent = Buffer.from(content, encoding).toString("utf-8")
if (fileContent.includes('abc')) {
// trigger file deletion
return deleteFile
}
// do not alter file content
return null;
}
}
|
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #129
I raised this PR to discuss some challenges I phased with TypeScript see comments.