You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ShellJS fails to remove asar archives when run in Electron due to
Electron fs patches that treat asar archives as virtual directories.
See issue #618 .
To overcome this, we set process.noAsar to true.
ShellJS fails to remove asar archives when run in Electron due to
Electron fs patches that treat asar archives as virtual directories.
See issue shelljs#618.
To overcome this, we set process.noAsar to true.
@tgds Thanks for the patch. I agree that electron support is something we need to work on, and I'd like to move toward that in future releases.
From the looks of it, this seems like something end users might prefer to do on their own. I have reservations about modifying globals, especially since ShellJS is often used with other packages. This has led in the past to some hard-to-find bugs.
Although we're resetting noAsar's value here, it's still not a total perfect reset, since things subtly change (fails 'key' in obj check):
>// in regular node>'noAsar'inprocessfalse>varnoAsar=process.noAsarundefined>process.noAsar=noAsarundefined>'noAsar'inprocess// this value has changed, which end users might not expecttrue
Even if we perfectly restore the state, there's always a chance that rm() will change in the future and might unexpectedly skip the cleanup steps, which would be hard to diagnose.
In the interest of kicking off support for electron, what if we instead document this in a new ShellJS wiki page, specifically for electron issues? That way ShellJS users know what to do and won't be surprised if this change breaks some other behavior.
@nfischer Yeah, certainly doesn't feel like the cleanest solution. Very much appreciate your feedback, I still need to develop the sense of knowing what one can safely afford to do.
How about requiring original-fs instead of fs when it's available? I know this would again be a little messy since we would be using slightly different fs library at one place.
Is there a reason why requiring users to use process.noAsar = true; before calling shell.rm() is not feasible?
Using original-fs might be the right approach. This approach is certainly safer, but we would probably need to add electron as an environment we test against in CI, which probably won't happen for a while.
Are there any scenarios where rm is useful if noAsar isn't modified? Also, does this asar stuff make cat, cp, etc. more useful or less useful? I wouldn't want to do anything until we're sure we aren't breaking a valid use case.
@tgds I started this wiki page for documenting electron support. I think I can hack something out pretty quickly to expose a workaround for exec(). If you find any other issues with electron support, let me know and I'll see if we can find reasonable workarounds.
Depending on how difficult electron support is, this may be slated for our v0.8 or v0.9 release, but we should probably stick with reasonable workarounds at least until then.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ShellJS fails to remove asar archives when run in Electron due to
Electron fs patches that treat asar archives as virtual directories.
See issue #618 .
To overcome this, we set process.noAsar to true.