Only use last line as fileName when packing #115
Conversation
src/lib/utils/pack.ts
Outdated
| @@ -48,7 +48,8 @@ export async function pack(srcDir: string, dstDir: string) { | |||
| ); | |||
| }); | |||
|
|
|||
| const fileName = path.basename(stdout.trim()); | |||
| const lastLine = stdout.trim().split("\n").pop() || ""; | |||
There was a problem hiding this comment.
I think it would be better to throw a descriptive error if lastLine is undefined, don't you think? Maybe even add the stdout to the message, like Failed to parse last line from stdout: ${stdout.trim()}
If basename can't parse the empty string, or if fileName becomes an empty string, the resulting error might not be so obvious.
Also, I have a slight preference for at(-1) instead of pop(), but that's nitpicking :)
There was a problem hiding this comment.
Agreed, updated now!
src/lib/utils/pack.ts
Outdated
| log.error( | ||
| `Failed to parse last line from stdout: ${stdout.trim()}` | ||
| ); | ||
| throw new Error("Unable to find fileName from pack output"); |
There was a problem hiding this comment.
I don't see a need to log and throw, as the thrown error message should appear anyway.
Please throw the message from log.error, as it is more informative.
There was a problem hiding this comment.
Not sure I understand exactly what you want me to do - only log, or only throw?
There was a problem hiding this comment.
Only throw, but throw an error with the message that you currently use in log.error. Thanks!
There was a problem hiding this comment.
... unless you can convince me that logging + throwing has a clear benefit. I never do that myself(*), but that doesn't mean I'm right necessarily ;)
(*) Unless the thrown error ends up being visible to the end-user and the system needs to log more/sensitive information internally.
There was a problem hiding this comment.
Gotcha, updated!
Well, I guess it depends on the error and log handling in general so I just did it both 😅
As you say, sometimes the error is shown to the user and the log message can contain more useful stuff for debugging.
There was a problem hiding this comment.
Please run prettier (pnpm format) to make the checks pass, then I can merge it...
There was a problem hiding this comment.
Done!
And thank you for your swift response, and efforts on the library in general 😊
|
@hjellek I've published it under @next. Maybe you can give it a try and confirm that it works for you? I will include it in firebase-tools-with-isolate when a new minor version of the upstream firebase-tools comes out. |
I'll be happy to test, but I can't find a branch or tag called |
|
@hjellek It's available on npm under @next. So something like |
Fixes #114