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: externalize dependencies by default during SSR #6698

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Collaborator

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 exports or module.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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@benmccann
Copy link
Collaborator Author

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

@brillout
Copy link
Contributor

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.

@benmccann
Copy link
Collaborator Author

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.

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

@ygj6
Copy link
Member

ygj6 commented May 5, 2022

I opened #8025 which should fix the failing test

@benmccann
Copy link
Collaborator Author

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

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.

3 participants