Skip to content

feat: allow function in noExternal#13568

Open
manucorporat wants to merge 3 commits into
vitejs:mainfrom
BuilderIO:feat-ssr-external-func
Open

feat: allow function in noExternal#13568
manucorporat wants to merge 3 commits into
vitejs:mainfrom
BuilderIO:feat-ssr-external-func

Conversation

@manucorporat

@manucorporat manucorporat commented Jun 20, 2023

Copy link
Copy Markdown
Contributor

Description

noExternal only supports array of string, or Regex, but this is not enough to more dynamic things like skipping noBuiltin node dependencies. In addition the underlining api from rollup allows to define the external configuration as a function

Additional context

Allow things like:

{
  noExternal: everythingButNodeBuiltins
}

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 PR Title 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.

@bolt-new-by-stackblitz

bolt-new-by-stackblitz Bot commented Jun 20, 2023

Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

patak-cat
patak-cat reviewed Jun 20, 2023
View reviewed changes
Comment thread pnpm-lock.yaml Outdated
@@ -1,4 +1,4 @@
lockfileVersion: '6.0'
lockfileVersion: '6.1'

@patak-cat patak-cat Jun 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lockfileVersion: '6.1'
lockfileVersion: '6.0'

@patak-cat patak-cat added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Jun 20, 2023
@patak-cat

patak-cat commented Jun 20, 2023

Copy link
Copy Markdown
Member

/ecosystem-ci run vitest

@vite-ecosystem-ci

vite-ecosystem-ci Bot commented Jun 20, 2023 •
edited
Loading

Copy link
Copy Markdown

📝 Ran ecosystem CI: Open

suite result
vitest ❌ failure

@patak-cat

patak-cat commented Jun 20, 2023

Copy link
Copy Markdown
Member

I think this is a good extension.

As @sheremet-va commented, it would break Vitest:

Error: packages/vite-node build: src/server.ts(57,34): error TS2345: Argument of type 'string | RegExp | ((id: string) => boolean)' is not assignable to parameter of type 'string | RegExp'.

But we had merged similar features broke Vitest before in minors. I'll run full ecosystem ci to see if there are other failures (Astro and Iles are expected to fail)

@patak-cat

patak-cat commented Jun 20, 2023

Copy link
Copy Markdown
Member

/ecosystem-ci run

@vite-ecosystem-ci

vite-ecosystem-ci Bot commented Jun 20, 2023 •
edited
Loading

Copy link
Copy Markdown

📝 Ran ecosystem CI: Open

suite result
analogjs ✅ success
astro ❌ failure
histoire ❌ failure
iles ❌ failure
ladle ✅ success
laravel ❌ failure
marko ❌ failure
nuxt ✅ success
nx ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
unocss ✅ success
vite-plugin-pwa ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ❌ failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patak-cat patak-cat patak-cat left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@manucorporat @patak-cat