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: special handling for ssr.noExternal in mergeConfig #4766

Merged
merged 1 commit into from
Aug 30, 2021

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Aug 27, 2021

Description

The new ssr.noExternal: true feature added recently offers a way to bundle all the dependencies. However, if a plugin also adds ssr.noExternal: ['one-dependency'] in its own config, Vite will give priority to the latter over the boolean because it has an object type.

This PR adds a special handling for noExternal key to avoid overriding the boolean value since it's actually a superset of any object value.

Additional context


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.

@patak-dev
Copy link
Member

@frandiox would you add an inline plugin in the playground where the ssr.noExternal: true is tested so we avoid future regressions?

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Aug 27, 2021
@frandiox
Copy link
Contributor Author

@patak-js Thanks, I've updated the commit!

@patak-dev patak-dev changed the title fix: Special handling for ssr.noExternal in mergeConfig fix: special handling for ssr.noExternal in mergeConfig Aug 30, 2021
@patak-dev patak-dev merged commit 689a2c8 into vitejs:main Aug 30, 2021
@jplhomer
Copy link
Contributor

jplhomer commented Sep 1, 2021

NICE thanks @frandiox

aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
@frandiox
Copy link
Contributor Author

@patak-dev @bluwy Hey 👋 it looks like this feature has been modified (maybe here)?

We are still doing noExternal: true in Hydrogen but a recent update in another package (Remix, added these lines) is making it fail because it now thinks these dependencies are external.

Just wanted to start the conversation to see if you have any workaround from the top of your mind. I can also submit a repro in a few days (traveling at the moment).

@hi-ogawa
Copy link
Collaborator

I think you can mutate the config after remix, something like this?

defineConfig({
  plugins: [
    remix(),
    {
      name: "workaround", 
      config(config) {
        delete config.ssr.external
      }
    }
  ]
})

@bluwy
Copy link
Member

bluwy commented Apr 24, 2024

Yeah @hi-ogawa's solution would be one way around it. For the linked remix code, perhaps you can also conditionally external them only in dev? (If it's a build issue).

The change for how noExternal and external interact was changed at #8348 where we want to respect explicitly set external config, because there's cases where some dependencies cannot be really bundled, e.g. contains binaries, so it needs to be externalized even with noExternal: true.

@frandiox
Copy link
Contributor Author

frandiox commented May 1, 2024

Remix fixed this in their latest release. However, I wonder how should we approach this if another user-land plugin adds something to external and it has priority over our noExternal.

For the linked remix code, perhaps you can also conditionally external them only in dev? (If it's a build issue).

It's also for dev because we were using the Runtime API to run on workerd.

The change for how noExternal and external interact was changed at #8348 where we want to respect explicitly set external config, because there's cases where some dependencies cannot be really bundled, e.g. contains binaries, so it needs to be externalized even with noExternal: true.

Yeah I see the utility. However, when targetting worker environments I think there's no actual way to make anything external 🤔
Maybe there could be an exception for the following combination?

ssr: {
  noExternal: true,
  target: 'webworker',
},

Or maybe there is if the worker supports imports... unsure.
In any case, probably low priority 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants