Skip to content
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

Fix #2088: unbreak automatic reinstall of linked dependencies on change #2091

Conversation

joaotavora
Copy link

@joaotavora joaotavora commented Dec 23, 2020

  • 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.

Changes

This fix bug #2088, a regression. It unbreaks the automatic reinstall of linked dependencies on change, which was lost after commit 610fb9d.

Testing

Not extensively tested I must say. I manually tested by changing the file of a single linked dependency. Anyway, the change to the code is small so it should be easy to understand what it does.

I also ran yarn test from the console. i got a single failure related to something react/svelte. I don't think i introduced that. Maybe I need some package installed or something.

Docs

bug fix only. Presumably this is already documented. But if it's not, tell me and I'll see to it. I write decent documentation.

@vercel
Copy link

vercel bot commented Dec 23, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/ib25ba2vb
✅ Preview: https://snowpack-git-fork-joaotavora-fix-fix-2088-unbreak-d0a4e7.pikapkg.vercel.app

[Deployment for 699a00e failed]

…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.
@joaotavora
Copy link
Author

I rebase this on top of main, but seems I broke some CI tests. Not sure what's up.

Otherwise it's really easy to blow up the FS watcher limit.

* snowpack/src/commands/dev.ts: (startServer::depWatcher): Also
ignore stuff in config.exclude.
This is done rather brutally by clearing all the cache, but could
probably be done more criteriously.

* snowpack/src/commands/dev.ts (startServer::handleRequest): Improve
debug log message.
(startServer::handleRequest): Clear knownETags.
@melissamcewen melissamcewen added this to Evaluating in Triage Mar 11, 2021
@melissamcewen melissamcewen moved this from Evaluating to Needs Triage in Triage Mar 11, 2021
@melissamcewen melissamcewen moved this from Evaluating PR to Ready for review in Triage Mar 12, 2021
@FredKSchott
Copy link
Owner

Thanks to work done in #2707 this is now handled automatically by Snowpack.

Triage automation moved this from Ready for review to Closed Mar 15, 2021
@joaotavora
Copy link
Author

Nice. I switched to Vite a long time ago, where this is handled perfectly, who knows maybe I'll try Snowpack again someday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants