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(import-analysis): skip import-analysis for external dependencies. #6583

Closed

Conversation

hasangenc0
Copy link

@hasangenc0 hasangenc0 commented Jan 21, 2022

Description

Resolves #6582

When I have external dependencies that are not an npm package vite dev server throws an error in vite:import-analysis stage. vite build command just works fine but vite command tries to resolve all of the external dependencies in the import-analysis stage.

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

Could you link or explain the issue you are trying to solve? Please also add a test to one of the playgrounds to avoid future regressions

@hasangenc0
Copy link
Author

Hi @patak-dev,
I already linked the issue under the pr description. The issue is #6582. I will add tests to a playground soon 👍

@bluwy bluwy mentioned this pull request Apr 3, 2022
4 tasks
@hasangenc0
Copy link
Author

#6582 (comment)

@patak-dev
Copy link
Member

I think it isn't bad to support this, at least to avoid the inconsistency between build and dev. @bluwy what do you think?

external could be something different than an array though. See

export type ExternalOption =
	| (string | RegExp)[]
	| string
	| RegExp
	| ((source: string, importer: string | undefined, isResolved: boolean) => boolean | null | void);

There should be code that we could grab from rollup to do this check.

An id could also be marked external by a plugin, like

function externalizeDependencyPlugin() {
  return {
    name: 'externalize-dependency',
    resolveId(source) {
      if (source === 'my-dependency') {
        return { id: 'my-dependency-develop', external: true };
      }
      return null;
    }
  };
}

But we could implement support for this separately (if we end up doing it).

@bluwy
Copy link
Member

bluwy commented May 11, 2022

Yeah I think this make sense too. And you're right with the complex types, Rollup has an API for that:

(await this.resolve(id))?.external

this.isExternal is deprecated.

@bluwy bluwy added p2-nice-to-have Not breaking anything but nice to have (priority) needs rebase labels May 11, 2022
@bluwy
Copy link
Member

bluwy commented Jun 23, 2022

Thanks for the PR @hasangenc0, but I don't think this is the right way to handle external in import-analysis. rollupOptions.external also accepts an array, regex, and function too, so this won't be handled correctly for those cases. Furthermore we should probably bail early for externalized imports instead of letting it flow through Vite plugins to be compatible with Rollup.

I've made a comment at #6582 (comment) with an early implementation of it. It's still missing tests and an API. Feel free to take over that branch if you'd like to implement it!

@bluwy bluwy closed this Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs test p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support external in dev
3 participants