-
Notifications
You must be signed in to change notification settings - Fork 6.3k
fix(lib/vscode): remove symlink in npm package #3935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
Merging #3935 (9d83659) into main (741b834) will not change coverage.
The diff coverage isn/a.
@@ Coverage Diff @@ ## main #3935 +/- ## ======================================= Coverage 63.51% 63.51% ======================================= Files 36 36 Lines 1872 1872 Branches 379 379 ======================================= Hits 1189 1189 Misses 580 580 Partials 103 103
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 741b834...9d83659. Read the comment docs.
5710b04 to
9d83659
Compare
@jawnsy
jawnsy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the writeup! I trust you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me, but I don't know what I don't know w.r.t to the fork solution and testing this.
the ripgrep ext worked, but I have no way to know if some other ext breaks :(
I don't know w.r.t to the fork solution and testing this.
I think @TeffenEllis mentioned once the fork solution is implemented, we won't even have to worry about this symlink issue cause it's non-existent there.
the ripgrep ext worked, but I have no way to know if some other ext breaks :(
Yeah, worked for me too! I guess we'll have to release and see what happens? If something does break, we can figure it out though. And eventually, we'll be able to write more tests for specific extensions (once #3621 gets merged)
Revert "Merge pull request #3935 from cdr/jsjoeio-rm-symlink"
Uh oh!
There was an error while loading. Please reload this page.
This PR removes the symlink to
node_modules.asarwhen packaging up fornpmbecause as of recently, npm no longer allows symbolic links in packages (we also don't need it).Why are we doing this?
On Friday, August 6th, we released 3.11.1.
The npm workflow did not work as expected leading to 3.11.1 not being published on npm, Homebrew, or AUR. We did some investigation and came to the conclusion something changed on the npm side with regard to symlinks in packages.
We spoke to someone today, August 9th from GitHub who confirmed our theory.
A few of the maintainers chatted and we came to the conclusion that we can remove this symlink.
—@code-asher
This PR removes that symlink.
Why did we have this before?
If you take a look at
lib.sh, you'll see previous maintainers left a comment explaining why we do this.You sure this won't break anything?
90% sure it shouldn't break anything because we actually remoe the symlink in the post-install and recreate it anyway so the one included in the npm package is never actually used.
There may be an edge case or two that we didn't anticipate but I (@jsjoeio) removed it, published to npm, tested locally and did not find any major issues.
How I tested locally
yarnyarn buildyarn build:vscodeyarn releasecd release && find . -type l -lsThen I tested the individual release:
cd releaseyarnnode .Screenshot and video
image
Screen.Recording.2021年08月09日.at.12.18.56.PM.mov
Fixes N/A
Related:
Other notes
@jawnsy showed me this trick to find symlinked files in a directory.
find . -type l -lsHelpful for debuggging this kind of thing.