[WIP] Fix bug where running script without extension removes existing .mjs files
#276
Conversation
|
I just filled out the CLA at https://cla.developers.google.com/. If a maintainer could re-run the check, that would be much appreciated. Thanks in advance! |
zx.mjs
Outdated
| @@ -101,10 +101,12 @@ async function writeAndImport(script, filepath, origin = filepath) { | |||
|
|
|||
| async function importPath(filepath, origin = filepath) { | |||
| let ext = extname(filepath) | |||
| let tmpFilename= Math.random().toString(36).substr(2) + '.mjs' | |||
There was a problem hiding this comment.
One thing to consider: error messages.
User will get errors with file name is random which can lead to bad DX.
What if filename + '.mjs' is not exists? We can use it (same as old behavior).
In case there is a collision use: filename + random + '.mjs'.
WDYF?
There was a problem hiding this comment.
Oh, good call. Yeah, that makes sense to me. Will push that change later today!
There was a problem hiding this comment.
Hi @antonmedv, sorry for the delay.
Just pushed the requested fix. If you feel like it's good to go, I'll let you hit resolve conversation. If you'd like me to make any additional fixes, though, just let me know. Thanks!
|
I'd suggestion solution like |
This PR fixes #208.
Description
Node cannot run scripts without an extension. To account for this, whenever a user would try to call
zxon a script without an extension,zxwould try to write the script to an.mjsfile before importing it. However, this caused a bug where if there was an existing.mjsfile with a similar name (e.g. user runs./hello, but there already exists a./hello.mjsin the same dir), the existing file would get overwritten and then removed.Changes
This PR fixes the issue by adjusting
importPathto use a uniquely generatedtmpFilename. This will ensure that if a user callszxusing a script without an extension, it will not inadvertently overwrite/remove a similarly named existing.mjsfile.Tests
This PR includes an update to the no extension test case (
test.mjs:83) to verify the fix worked as intended.Checklist