mirror of
https://github.com/jehna/humanify.git
synced 2026-04-27 17:45:58 +03:00
[GH-ISSUE #54] Better error handling when humanify local ran on empty file (Error: Failed to stringify code) #29
Labels
No labels
bug
enhancement
pull-request
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/humanify#29
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @0xdevalias on GitHub (Aug 23, 2024).
Original GitHub issue: https://github.com/jehna/humanify/issues/54
@0xdevalias commented on GitHub (Sep 26, 2024):
Turns out that this issue is the same root cause:
Which I debugged and identified the root cause + suggested potential places to fix it here:
@0xdevalias commented on GitHub (Sep 26, 2024):
Hacky workaround:
@0xdevalias commented on GitHub (Sep 29, 2024):
I believe this PR by @j4k0xb will also technically resolve this issue; though I wonder if it would be better UX to detect the empty file earlier on at the command level, and provide a more 'human focussed' error/warning at that stage as well:
The nuanced difference between this issue and the original bug in #111 is that this issue is about starting with an empty file, whereas #111 is about
webcrackunbundling creating empty files that then breakhumanifylater on when it tries to run over them; which is what #134 specifically fixes/works around.@j4k0xb commented on GitHub (Sep 29, 2024):
An empty file is valid JavaScript so there shouldn't be any warning/error imo.
That's what most CLIs (formatters, bundlers, etc) do.
@0xdevalias commented on GitHub (Sep 29, 2024):
@j4k0xb I'm specifically talking about the human focussed error/warning when running this against an empty starting file, and providing better feedback to the user around that. If needs be you could add an escape hatch of
--allow-empty-filesor similar if there was really a need to; but I don't think showing an extra UX focussed note in the log output is a big issue.And then similarly for empty files when they have been unbundled, providing that feedback in the logs (or perhaps only verbose logs) can be useful.
Like, these tiny UX focussed tweaks I made to what was displayed in the output logs (ignoring the 'skip to file' part) made a MASSIVE difference to helping track down and identify where/why a bug occurred, or why the program seemed to be hanging so long/similar:
@j4k0xb commented on GitHub (Sep 29, 2024):
True, more verbose code and progress logging seems useful
But still not a fan of skipping files...
require("./empty.js")which get's removed because of this?@0xdevalias commented on GitHub (Sep 29, 2024):
@j4k0xb I guess that really depends where 'skipping' is implemented. The hacky workaround in
unminify(Ref) was never suggested as a proper fix to this, it was only a hacky workaround, as I said at the bottom of it:But for example, because
unminifyreduces over the passed in plugins, and feeds the code into them; theopenaiRenameplugin:github.com/jehna/humanify@64a1b9511d/src/plugins/openai/openai-rename.ts (L6-L20)Or even
visitAllIdentifiersitself:github.com/jehna/humanify@64a1b9511d/src/plugins/local-llm-rename/visit-all-identifiers.ts (L15-L25)Could have an 'early guard statement' when
codeis empty, that provides some feedback and thenreturnsbefore even needing to attempt to parse the AST.My suggestion is about identifying the appropriate place to check that the inputs are relevant, and react appropriately if not. There's no reason to parse the AST and have all the other code in that file run just to turn it back into an empty string if we can just handle that case specifically at the beginning. While it's potentially a negligible performance gain, at the very least it makes the handling explicit, and makes the code easier to reason about/less chances for bugs to occur because of it.
Like without thinking too deeply about it, I would probably implement something like this:
And because
visitAllIdentifiersdoesn't seem to currently have any patterns around logging, to keep with that, I would also make a change inopenaiRename(and other similar plugins) like this:@0xdevalias commented on GitHub (Sep 29, 2024):
In addition to the suggestions in my last comment, I was also going to suggest adding a warning/error at the command level, but in looking at the code again now, I don't think that makes sense:
github.com/jehna/humanify@64a1b9511d/src/commands/openai.ts (L20-L30)Basically, the
openaicommand is just callingunminify, and it's withinunminifythat the file is first actually read, before being passed towebcrack:github.com/jehna/humanify@64a1b9511d/src/unminify.ts (L6-L13)So the only way I think it would make sense to implement anything new at the command level is if
unminifywas refactored to instead acceptcoderather thanfilename; but at this stage, I'm not really sure if there is any added benefit to doing that; unless it becomes useful to do so as part of the implementation for issues like:So I think just the 2 minimal suggestions I made in https://github.com/jehna/humanify/issues/54#issuecomment-2381073944 + the fix in https://github.com/jehna/humanify/pull/134 would probably satisfy my requests on this issue.
@0xdevalias commented on GitHub (Oct 15, 2024):
Looks like that hacky workaround (rather than the better full solution mentioned above) was what @jehna ended up implementing in #134 in the end though.
github.com/j4k0xb/humanify@96c093bc99/src/unminify.ts (L21-L24)@jehna commented on GitHub (Oct 18, 2024):
This should be fixed now
@0xdevalias commented on GitHub (Oct 20, 2024):
@jehna Fixed technically, but see notes above in https://github.com/jehna/humanify/issues/54#issuecomment-2412626831 + https://github.com/jehna/humanify/issues/54#issuecomment-2381073944 for why the way it was implemented may not have been the ideal place to do so; and what a more flexible option would have been.