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

When bundling, conditional exports in package.json are selected incorrectly #3772

Closed
6 tasks done
JBusillo opened this issue Jun 12, 2021 · 9 comments
Closed
6 tasks done

Comments

@JBusillo
Copy link
Contributor

Describe the bug

When importing a module that has conditional exports specified (e.g., "browser", "import", "require" or "node"), Vite may select the incorrect export, depending on the order of exports in the module's package.json file.

For js/ts files that are strictly used for endpoints (e.g., target of a fetch request), this is problematic. The specific case is when importing "jose" to sign JWTs. In its package.json, you can see the "exports" , of which many have "browser", "import" and "require" entries. Example:

 "exports": {    
    "./package.json": "./package.json",
    "./jwe/compact/decrypt": {
          "browser": "./dist/browser/jwe/compact/decrypt.js",
          "import": "./dist/node/esm/jwe/compact/decrypt.js",
          "require": "./dist/node/cjs/jwe/compact/decrypt.js"
    }, ...

Resolving these exports is handled by lukeed/resolve.exports. The order is respected, so in the above case, "browser" will always be selected. Since the endpoint code is run via "node", the selected module fails -- in this case, the error is "vite_ssr_import_1.default.btoa is not a function".

This might be related to #3736, except that it involves Webpack, and that the culprit (okta-auth-js) doesn't have conditional exports in its package.json.

Reproduction

Here is the repo

After cloning, do an "npm i" and a "npm run dev"
This will capture the vite log into vite-debug.txt in the root directory
You'll see the error at the bottom, and if you do a search for "jose", you'll see that the "browser" version was imported.

System Info

 System:
    OS: Linux 5.8 Ubuntu 20.04.2 LTS (Focal Fossa)
    CPU: (6) x64 Intel(R) Core(TM) i5-8400 CPU @ 2.80GHz
    Memory: 14.00 GB / 31.04 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 14.17.0 - /usr/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 7.12.0 - /usr/local/bin/npm
  Browsers:
    Brave Browser: 91.1.25.68
    Chrome: 91.0.4472.77
    Firefox: 89.0

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

  • Read the Contributing Guidelines.
  • Read the docs.
  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • Provide a description in this issue that describes the bug.
  • Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
  • Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
@hgl
Copy link
Contributor

hgl commented Jun 17, 2021

I think this is the cause:

First, vite always tries to import the browser version of what's in the exports, without considering SSR:

return _resolveExports(pkg, key, {
browser: true,
require: options.isRequire,
conditions

Once that's fixed, something else goes wrong:

When trying to importjose/jwt/sign in SSR, tryNodeResolve is executed, and then it executes:

// this is a missing import.
// queue optimize-deps re-run.
server._registerMissingImport?.(id, resolved)

However It never passes the SSR flag. After a deep stack of calls (e.g., esbuildDepPlugin's resolver call), resolveId hook in the resolve plugin is re-entered trying to import crypto, with the ssr argument being undefined, so this check fails:

if (isBuiltin(id)) {
if (ssr) {
return {
id,
external: true
}

And the browser version of crypto is then imported.

I'd be happy to send a PR, but I'm not sure how to pass the SSR flag deeply without littering methods with arguments, so esbuildDepPlugin's resolver call can pass along that flag to the resolve plugin's resolveId hook. I'd appreciate it if someone familiar with vite internals could point me in the right direction.

@patak-dev
Copy link
Member

@hgl great digging. esbuildDepPlugin gets the resolved config, so you can access config.build.ssr inside the plugin.

@hgl
Copy link
Contributor

hgl commented Jun 18, 2021

@patak-js Thanks for the help.

From my testing, it seems config.build.ssr is always false for SSR.

And from the code, it got the initial value from here:

lib: false,
ssr: false,
ssrManifest: false,

and later an additional SSR flag is passed alongside it when the logic enters SSR:

const result =
mod.ssrTransformResult ||
(await transformRequest(url, server, { ssr: true }))

Maybe I missed the code where config.build.ssr is changed? But even if I did, config.build.ssr in esbuildDepPlugin was false for my SSR request.

@patak-dev
Copy link
Member

ssr: false in config.build.ssr is set here

But you are right, this flag means you are compiling the ssr build, not that you are running the server. Looks like there isn't a way to know if you are doing ssr from the config. Maybe we could use config.server.middlewareMode === 'ssr'? I don't see any usage in the code base because middlewareMode was a boolean and we changed it recently to allow the usage of the HTML plugin... it was not the intention of this flag to state that you are in ssr though, but that you want to control how the server works.
Seems safer to pass the ssr flag around. @brillout, what do you think?

@brillout
Copy link
Contributor

Hm I'm not sure; I'm not all too familiar with the source code but yes seems like

/**
* Create Vite dev server to be used as a middleware in an existing server
*/
middlewareMode?: boolean | 'html' | 'ssr'

to be what we are looking for.

So I guess SSR <=> assert(config.build.ssr || config.server.middlewareMode === 'ssr').

@patak-dev
Copy link
Member

Correct me if I'm wrong, but config.build.ssr (or using the --ssr cli option) applies only during building of the SSR entry.

And for the SSR server, I don't think we can use config.server.middlewareMode === 'ssr' here. In the docs we say:

When building an SSR app, you likely want to have full control over your main server and decouple Vite from the production environment. It is therefore recommended to use Vite in middleware mode.

Even if it is recommended, the user may chose to have a full server, or use Vite in middlewareMode === 'html'. I now think that the name 'ssr' is not the best for middlewareMode as it may cause confusion.

So looks like we need to pass around the ssr param that we get in plugins, or propose a new config option for resolved like config.server.ssr that users needs to always set if they want to use the server for SSR (because this is a breaking change, I don't think we'll be doing this any time soon).

Another idea would be to have a WeakMap: config -> ssr, and set when entering ssrLoadModule, and remove the entry when it is finished. So we can know if this is a ssr transform in the plugins from the config instead of having to pass the flag around. Abstracted in a isSsrServer(config). For the moment, I think we should just use the ssr flag passed to the hooks.

@hgl
Copy link
Contributor

hgl commented Jun 19, 2021

tryNodeResolve turns the SSR flag into targetWeb, If we're going to pass the flag around, should it accept another argument for the original flag?

And these methods need to accept the new argument, if I'm not missing anything:

  1. server._registerMissingImport
  2. optimizeDeps
  3. esbuildDepPlugin
  4. esbuildDepPlugin's resolver

Will the PR be accepted if it needs to touch this amount of methods?

@patak-dev
Copy link
Member

I think we should check it in the PR review with other maintainers, as it will be easier to discuss seeing the changeset. At least to me, this sounds ok.

@github-actions
Copy link

This issue gets locked because it has been closed for more than 14 days.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this issue Nov 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants