search

lucacasonato / esbuild_deno_loader

Deno module resolution for `esbuild`
https://jsr.io/@luca/esbuild-deno-loader
MIT License
176 stars 46 forks source link

fix: Move temp folder instead of renaming #83

Closed mxdvl closed 10 months ago

mxdvl commented 1 year ago

What does this change?

Use move method from std/fs, which returns the correct errors for existing files.

Why?

Fixes #82

There may be a deeper issue, which is that we do not get the correct underlying error from the filesystem:

https://github.com/denoland/deno_core/blob/69891d0685d8e0ab5d80d6e3204c241796046a30/core/error_codes.rs#L50C52-L50C52

mxdvl commented 11 months ago

Any chance you could get a look @lucacasonato?

mxdvl commented 11 months ago

Thanks @marvinhagemeister, wasn’t sure if you were a maintainer as well! I think someone will need to approve the actions for them to run on this PR. Alternatively, recreating my branch in this repo–as opposed to the fork–should also trigger them.

marvinhagemeister commented 11 months ago

@lucacasonato Do you have an idea why the CI is not getting triggered?

mxdvl commented 11 months ago

@lucacasonato Do you have an idea why the CI is not getting triggered?

If there’s no button to “approve workflows” as per the docs, I think the second-best approach is to own the changes yourselves. You could achieve this by copying the mxdvl:patch-1 branch over to this repo.

Feel free to recreate this PR or cherry-pick those commits yourselves, by the way, authorhip of the change is less important that the fix actually going out! 🙇

mxdvl commented 10 months ago

All green now. I don't have the power to merge so either of you will have to do it.

marvinhagemeister commented 10 months ago

I'm currently investigating this. We found a reproduction case, but the changes in this PR don't seem to fully fix it.

lucacasonato commented 10 months ago

Fixed by #107