fix: externalize dependencies by default during SSR#6698
fix: externalize dependencies by default during SSR#6698benmccann wants to merge 1 commit intovitejs:mainfrom
Conversation
|
Hmmm. The tests are failing, which I didn't expect. I still think this is probably a good idea, but it's not a high priority for me, so I'm just going to go ahead and close this. Perhaps @brillout or someone else would like to take a stab at it in the future in which case I'll give a review and likely approval to any PR |
|
I had a quick look at the failing test, I couldn't see how it's related to this PR. Maybe it was just a flaky test? Thinking maybe rebasing this would fix the test. Because, in principle, I also (still) see this as a good idea. |
Rebasing isn't enough and the test isn't flaky, but fails consistently. I did investigate some awhile ago and sent #7332 to clarify the code comments in that test to make it easier to understand what it's supposed to be testing and what's supposed to be happening. I'm not sure why it's failing though and haven't really had time to dig in |
|
I opened #8025 which should fix the failing test |
|
That's amazing! Thank you so much! I couldn't reopen this PR so sent a new one and all the tests are passing now: #8033 |
Description
Externalize dependencies by default during SSR
Additional context
We had talked about doing this in #5544. At the time, I didn't want to change the code more than necessary because we were trying to get 2.7 out the door and we had already made so many SSR changes. But now that SSR has settled down a bit, I do think it'd be a good cleanup to make.
As far as I'm aware, there's really nothing to gain by bundling rather than externalizing. I'm not aware of any case where that makes something work that otherwise wouldn't work. Rather, I'm only aware of cases where that breaks things because of incorrect or incomplete bundling logic (e.g. #2579)
One of our users recently reported a problem using Reflect.js. From a quick glance, it looks like the library is essentially a polyfill for a proposed piece of functionality to the ES spec, so it sticks its functionality onto the global scope and doesn't export anything. Because it doesn't export anything, we don't see
exportsormodule.exports, etc. As a result, we fail to detect that the library is a CJS library and thus fail to externalize the library.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).