-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
feat: hide optimized deps found during scan phase logs #7419
Conversation
I agree with moving the logging behind debug flag. My one question about adding logging with |
I feel like we should still keep the logs, so that the user knows which imports triggered the on-the-fly bundling. And I think frameworks or end-users should try to keep on-the-fly bundling to the lowest too (ideally bundles once on startup), so having these kind of verbose logs could signal them to improve their setup, though I'm not sure why Ladle is behaving that way currently that causes the multi bundling. |
This is a good point. We should give some feedback when using force though. So maybe we should only log the first optimized deps batch with a message like:
But the rest of on-the-fly messages should probably also appear... because they also are there because you used
Ladle and similar projects have so many on-the-fly bundling because they don't scan the whole app upfront IIUC. Each time you click on a story, new dependencies are discovered. Maybe they should change this. But now that on-the-fly bundling isn't blocking, it may not be an anti-pattern anymore... the only issue is when these dependencies generate a page reload (and it is only the first time). We are showing a message in this case, and we could show something like:
Without a message like this, even if we show the deps logs, the user will not know how to fix it. IMO, if they see Maybe just this:
My vote still goes to hide the logs in the general case, but I see the value on bumping the user to avoid unneeded page reloads. |
Why can't dependencies be discovered via dependency scanning when using Ladle? Do they manually disable Vite's dependency scanning? Adding dependencies manually to |
Maybe @tajo could explain a bit more why these dependencies are discovered late. Maybe ladle could add the proper entries to I would still move forward with hiding as much of the deps logs as possible, and only show them when there is a reload (maybe with extra information about how to improve things, but looks quite hard to log something concise...). |
Yes, the storybook vite builder does attempt to use |
I think even if we report using Ultimately, if we could prevent multiple on-the-fly bundling by encouraging proper initial bundling, that would help the end-user for faster web page TTI and reduced repeated esbuild bundling, which is an overall plus. Hiding the logs would also hide this potential optimization IMO, unless we can provide correct recommendation in the logs) |
For the record, looks like vite-plugin-autoimport also ends up generating these late discovered deps: unplugin/unplugin-auto-import#124 @bluwy would you be ok if we remove the log for the first batch scanned deps and only log late discovered deps with a message like:
And if a page reload
So at least we don't have logs if every deps is discovered by the scan phase? I still think that it may be quite hard for the user to understand what is going on, but this would work for me |
Yep that sounds fine by me 👍 I dont usually find the initial scanned deps to be important too. |
I think it's fine to hide most of these logs as long as it can be enabled for debugging. Maybe @tajo could explain a bit more why these dependencies are discovered late. It's because stories are loaded as const Story = React.lazy(() => import('./foo.stories.js')); So the dependencies inside of foo.stories.js are not discovered initially, right? I tried to add all stories into optimizeDeps.entries to force all deps being pre-bundled upfront but it wasn't quite working, it still did some late discoveries. (I could try to create a repro for that and test it again with 2.9) Ideally, it would be nice to eliminate any reloads since it's not a great experience while keeping the startup instant no matter how many stories and dependencies there are. Maybe with 2.9, it's actually ok to put everything into include/entries. Otherwise, there could be an option like optimizeDeps.entriesPreload - Vite could defer pre-bundling of these deps until the initial phase is done. This would keep the startup fast while all other dependencies would get pre-bundled in background before user clicks on another story. |
👍 The only nice part of seeing them was knowing whether it was a hot or cold start, and now that the distinction doesn't matter as much, I agree that it doesn't really help to see the list of optimized dependencies. |
sounds good to me. I might tweak the language a bit to something like:
|
The thing about |
@tajo I think a repro is a good idea here, IMO we should be able to discover this. Lets create an issue and see if we can find the problem |
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.
I didn't review the code super closely, but leaving an approval here in terms of the behavior and log messages
colors.green( | ||
`✨ previous optimized dependencies have changed, reloading page` | ||
), | ||
colors.green(`✨ optimized dependencies changed. reloading`), |
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.
Maybe we could use a different emoji for this one. Like 🔄 or ⚙️. Not convinced by other options, probably better to keep the current one
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Description
As part of #7379, we modified the logs to make them more concise now that the user will generally not notice the extra time to scan and pre-bundle during cold start.
It now looks like this:
The logs can still be a bit noisy for projects like StoryBook, Cypress, Ladle, etc where there are a lot of newly discovered deps as the use explores the app:
IMO the only important log here is the last one. Since there is a page reload, we need to communicate to the user why we are doing so. But the rest of the logs feel more like implementation details to me. It isn't that useful for the user to know when a dependency has been pre-bundled if there is not a big noticeable delay in server start or during a rerun.
This PR hides these logs, and only shows them when debugging or when using
--force
. I think that having them when using force is a good idea since the user is focused on re-bundling the dependencies so it is good feedback in that case.What is the purpose of this pull request?