Fixed inconsistency for unlinking of pre-symlinked deps, and added option to toggle it #13
Conversation
This file contains 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
… folders get unlinked -- otherwise the npm install command deletes subdependencies in the symlinked folders that are shared by the root project. * Fixed that SymlinkCollector would keep tunneling deeper into all subfolders, when the root folder's path contains the "@" character. (now it only tunnels deeper when the folder name itself starts with "@" -- intended for packages hosted under an "@"-prefixed organization folder, apparently) * Fixed bug regarding the async/await in SymlinkCollector.execute, whereby the processing of subfolders would not be "awaited" for by the root execute call -- thus their results were being ignored.
…k/") were not getting restored properly. (it would try to restore "package-name" by itself, instead of "@org-name/package-name") * Made so that if command fails, it logs the actual reason/error! * Made so that if there are no packages to relink after the "npm install" command, the "npm link" command is not even run. (else, the "npm link X Y Z" is just "npm link", which has a different and unexpected behavior) * Removed unused "cb" param from SymlinkCollector.execute. (each call has apparently just been using the attached-to-instance "this.callback")
…npm install), if there was no ".nsi.json" file. * Added new option "keep-prelinked". This option tells nsi to not unlink any pre-symlinked dependencies found. Benefits of using the new option: 1) Increases the speed of nsi. (since the "npm install" command doesn't need to freshly-install those libraries) 2) Avoids the need for some IDEs to reload. (some IDEs only notice the file deletions, not their re-adding through new symlinks at the end) However, the option also has this major negative: 1) Because the pre-symlinked folders are not unlinked prior to "npm install", the npm-install process can (and will -- at least in some cases) tunnel into those symlinked folders and modify them (since it doesn't fully account for their being in external folders). Depending on the dependency-trees between the user project, and the symlinked folders, this can cause a number of subtle errors later on. (as I've experienced multiple times) Because of that one major negative, I currently have "keep-prelinked" disabled by default. If the repo owner (@UD_UD) wants this option enabled by default, then I think the Readme should at least contain mention of the potential drawbacks of it. (I just got bit by it this morning, hence my making the changes, and putting it into a pull-request)
|
Will review this. |
…exist (to be cleaned up), NSI would error.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Benefits of using the new option:
However, the option also has this major negative:
Because of that one major negative, I currently have "keep-prelinked" disabled by default.
@UD-UD If you'd like this option enabled by default, let me know, and I'll make the change. However, in that case I think the Readme should at least contain mention of the potential drawbacks of it. (I just got bit by it this morning, hence my making the changes, and putting it into a pull-request)