-
Notifications
You must be signed in to change notification settings - Fork 922
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
Add back support to reinstall linked dependencies on change #2088
Comments
I'm working on a fix right now :-) Having fun, but I won't be suprised if you beat me to it, since they'll be my first ever lines of OSS javascript/typescript. |
Nice! Let me know how I can help. That commit should include the old implementation of reinstallDependencies(), which may need to change a bit since it's so old now, but should have enough to point you in the right direction. Appreciate the help! |
I'm making progress, indeed. I'll let you know if I am blocked. First I'll try to get back that function you mention and get this working the way it presumably used to. After that is done, I was thinking of being a little more ambitious and installing only the dependency that we know has changed, since I think |
…ies on change * snowpack/src/commands/dev.ts (installDependencies): import it. (onDepWatchEvent): Use it. * snowpack/src/commands/install.ts (InstallRunResult): export it. * snowpack/src/sources/local.ts (InstallRunResult): import it. (installDependencies): export it.
After much rummaging around in the code, I've settled for exporting and re-using an function in |
@FredKSchott what's the status on this? Do you think the fix I'm proposing will get merged in the near future, or do you have something else in mind? I think something more elaborate and/or efficient is clearly possible, but at least merging my fix seems to bring back the previous behavior. |
Sorry for the delay on this, I've been on holiday and then immediately went heads down into getting v3.0 out the door. Should have more time to take a look at this in the next couple of days. I can also help you rebase your PR, given that a few things had to move around for v3.0 since you started on it. |
…ies on change * snowpack/src/commands/dev.ts (installDependencies): import it. (onDepWatchEvent): Call it. * snowpack/src/sources/local-install.ts (InstallRunResult): export it. * snowpack/src/sources/local.ts (InstallRunResult): import it. (installDependencies): export it and give it a type.
I think I just did that, rebase on top of It's working fine, as far as I can tell, though on upgrading Snowpack to main I had to |
By the way, I found and fixed another bug. When making the FS watcher for linked deps, better consider |
Is there currently a workaround for this? |
Any word on this? it appears to have been ready for quite some time now |
According to #2091 (comment), this is supposed to be fixed now (with another change, not my PR). I wouldn't know, I haven't used Snowpack for some time :-/. But maybe this can be closed? |
I'm finding that not only are changes in linked dependencies not detected, but Snowpack can't build (at all) with any linked dependencies involved. Have tried clearing the cache and using
This one is pretty important to my development workflow, anything we can do to help? |
Any updates on this? I also can't use linked packages on latest Snowpack. |
See my comment #2088 (comment). It's supposed to be fixed ! |
Thanks @joaotavora . I am using snowpack version 3.8.2 and the issue is still present so I don't think is fixed yet. |
Original Discussion: #429
/cc @joaotavora
Regression:
The text was updated successfully, but these errors were encountered: